jail: consistently populate the KP_JID and KP_NAME parameters

The gaps here, specifically, were:
 - When we have to discover a running jail's jid from name, we should
    populate the missing jid param
 - When we populate jid/name from the config, if the name is a jid we
    wouldn't populate the name; now we do both.
 - When we create a jail, we should populate jid and name with whatever
    details we have now that we didn't both.

As a consequence, we can cleanup a few things:
 - vnet.interface and zfs.dataset can just always use the jid
 - Trying to populate JNAME should always work now, where it would be
    a little crashy before if you create a jail that didn't have a name
    or jid on the command line
 - We can simplify the just-prior JID population now that we'll keep a
    stringified jid in our intparams.

This primarily fixes the below, but the issues with vnet.interface and
zfs.dataset were pre-existing.

Fixes:	d8f021add4 ("jail: add JID, JNAME and JPATH to env [...]")
Reviewed by:	jamie
Differential Revision:	https://reviews.freebsd.org/D51502

usr.sbin/jail/command.c

@@ -290,7 +290,7 @@ run_command(struct cfjail *j)
 	const struct cfstring *comstring, *s;
 	login_cap_t *lcap;
 	const char **argv;
-	char *acs, *ajidstr, *cs, *comcs, *devpath;
+	char *acs, *cs, *comcs, *devpath;
 	const char *jidstr, *conslog, *fmt, *path, *ruleset, *term, *username;
 	enum intparam comparam;
 	size_t comlen, ret;
@@ -332,6 +332,25 @@ run_command(struct cfjail *j)
 				printf("%d\n", j->jid);
 			if (verbose >= 0 && (j->name || verbose > 0))
 				jail_note(j, "created\n");
+
+			/*
+			 * Populate our jid and name parameters if they were not
+			 * provided.  This simplifies later logic that wants to
+			 * use the jid or name to be able to do so reliably.
+			 */
+			if (j->intparams[KP_JID] == NULL) {
+				char ljidstr[16];
+
+				(void)snprintf(ljidstr, sizeof(ljidstr), "%d",
+				    j->jid);
+				add_param(j, NULL, KP_JID, ljidstr);
+			}
+
+			/* This matches the kernel behavior. */
+			if (j->intparams[KP_NAME] == NULL)
+				add_param(j, j->intparams[KP_JID], KP_NAME,
+				    NULL);
+
 			dep_done(j, DF_LIGHT);
 		}
 		return 0;
@@ -456,8 +475,7 @@ run_command(struct cfjail *j)
 		argv[0] = _PATH_IFCONFIG;
 		argv[1] = comstring->s;
 		argv[2] = down ? "-vnet" : "vnet";
-		jidstr = string_param(j->intparams[KP_JID]);
-		argv[3] = jidstr ? jidstr : string_param(j->intparams[KP_NAME]);
+		argv[3] = string_param(j->intparams[KP_JID]);
 		argv[4] = NULL;
 		break;
 
@@ -592,9 +610,7 @@ run_command(struct cfjail *j)
 
 	case IP_ZFS_DATASET:
 		argv = alloca(4 * sizeof(char *));
-		jidstr = string_param(j->intparams[KP_JID]) ?
-		    string_param(j->intparams[KP_JID]) :
-		    string_param(j->intparams[KP_NAME]);
+		jidstr = string_param(j->intparams[KP_JID]);
 		fmt = "if [ $(/sbin/zfs get -H -o value jailed %s) = on ]; then /sbin/zfs jail %s %s || echo error, attaching %s to jail %s failed; else echo error, you need to set jailed=on for dataset %s; fi";
 		comlen = strlen(fmt)
 		    + 2 * strlen(jidstr)
@@ -796,14 +812,10 @@ run_command(struct cfjail *j)
 		endpwent();
 	}
 	if (!injail) {
-		if (asprintf(&ajidstr, "%d", j->jid) == -1) {
-			jail_warnx(j, "asprintf jid=%d: %s", j->jid,
-				strerror(errno));
-			exit(1);
-		}
-		setenv("JID", ajidstr, 1);
-		free(ajidstr);
+		if (string_param(j->intparams[KP_JID]))
+			setenv("JID", string_param(j->intparams[KP_JID]), 1);
 		setenv("JNAME", string_param(j->intparams[KP_NAME]), 1);
+
 		path = string_param(j->intparams[KP_PATH]);
 		setenv("JPATH", path ? path : "", 1);
 	}

usr.sbin/jail/config.c

@@ -156,11 +156,14 @@ load_config(const char *cfname)
 		TAILQ_CONCAT(&opp, &j->params, tq);
 		/*
 		 * The jail name implies its "name" or "jid" parameter,
-		 * though they may also be explicitly set later on.
+		 * though they may also be explicitly set later on.  After we
+		 * collect other parameters, we'll go back and ensure they're
+		 * both set if we need to do so here.
 		 */
 		add_param(j, NULL,
 		    strtol(j->name, &ep, 10) && !*ep ? KP_JID : KP_NAME,
 		    j->name);
+
 		/*
 		 * Collect parameters for the jail, global parameters/variables,
 		 * and any matching wildcard jails.
@@ -180,6 +183,14 @@ load_config(const char *cfname)
 			TAILQ_FOREACH(p, &opp, tq)
 				add_param(j, p, 0, NULL);
 
+		/*
+		 * We only backfill if it's the name that wasn't set; if it was
+		 * the jid, we can assume that will be populated later when the
+		 * jail is created or found.
+		 */
+		if (j->intparams[KP_NAME] == NULL)
+			add_param(j, j->intparams[KP_JID], KP_NAME, NULL);
+
 		/* Resolve any variable substitutions. */
 		pgen = 0;
 		TAILQ_FOREACH(p, &j->params, tq) {

usr.sbin/jail/jail.c

@@ -890,7 +890,14 @@ running_jid(struct cfjail *j)
 		j->jid = -1;
 		return;
 	}
+
 	j->jid = jail_get(jiov, 2, 0);
+	if (j->jid > 0 && j->intparams[KP_JID] == NULL) {
+		char jidstr[16];
+
+		(void)snprintf(jidstr, sizeof(jidstr), "%d", j->jid);
+		add_param(j, NULL, KP_JID, jidstr);
+	}
 }
 
 static void

usr.sbin/jail/tests/jail_basic_test.sh

@@ -165,6 +165,133 @@ commands_cleanup()
 	fi
 }
 
+atf_test_case "jid_name_set" "cleanup"
+jid_name_set_head()
+{
+	atf_set descr 'Test that one can set both the jid and name in a config file'
+	atf_set require.user root
+}
+
+find_unused_jid()
+{
+	: ${JAIL_MAX=999999}
+
+	# We'll start at a higher jid number and roll through the space until
+	# we find one that isn't taken.  We start high to avoid racing parallel
+	# activity for the 'next available', though ideally we don't have a lot
+	# of parallel jail activity like that.
+	jid=5309
+	while jls -cj "$jid"; do
+		if [ "$jid" -eq "$JAIL_MAX" ]; then
+			atf_skip "System has too many jail, cannot find free slot"
+		fi
+
+		jid=$((jid + 1))
+	done
+
+	echo "$jid" | tee -a jails.lst
+}
+clean_jails()
+{
+	if [ ! -s jails.lst ]; then
+		return 0
+	fi
+
+	while read jail; do
+		if jls -e -j "$jail"; then
+			jail -r "$jail"
+		fi
+	done < jails.lst
+}
+
+jid_name_set_body()
+{
+	local jid=$(find_unused_jid)
+
+	echo "basejail" >> jails.lst
+	echo "$jid { name = basejail; persist; }" > jail.conf
+	atf_check -o match:"$jid: created" jail -f jail.conf -c "$jid"
+	atf_check -o match:"$jid: removed" jail -f jail.conf -r "$jid"
+
+	echo "basejail { jid = $jid; persist; }" > jail.conf
+	atf_check -o match:"basejail: created" jail -f jail.conf -c basejail
+	atf_check -o match:"basejail: removed" jail -f jail.conf -r basejail
+}
+
+jid_name_set_cleanup()
+{
+	clean_jails
+}
+
+atf_test_case "param_consistency" "cleanup"
+param_consistency_head()
+{
+	atf_set descr 'Test for consistency in jid/name params being set implicitly'
+	atf_set require.user root
+}
+
+param_consistency_body()
+{
+	local iface jid
+
+	echo "basejail" >> jails.lst
+
+	# Most basic test: exec.poststart running a command without a jail
+	# config.  This would previously crash as we only had the jid and name
+	# as populated at creation time.
+	atf_check jail -c path=/ exec.poststart="true" command=/usr/bin/true
+
+	iface=$(ifconfig lo create)
+	atf_check test -n "$iface"
+	echo "$iface" >> interfaces.lst
+
+	# Now do it again but exercising IP_VNET_INTERFACE, which is an
+	# implied command that wants to use the jid or name.  This would crash
+	# as neither KP_JID or KP_NAME are populated when a jail is created,
+	# just as above- just at a different spot.
+	atf_check jail -c \
+		path=/ vnet=new vnet.interface="$iface" command=/usr/bin/true
+
+	# Test that a jail that we only know by name will have its jid resolved
+	# and added to its param set.
+	echo "basejail {path = /; exec.prestop = 'echo STOP'; persist; }" > jail.conf
+
+	atf_check -o ignore jail -f jail.conf -c basejail
+	atf_check -o match:"STOP" jail -f jail.conf -r basejail
+
+	# Do the same sequence as above, but use a jail with a jid-ish name.
+	jid=$(find_unused_jid)
+	echo "$jid {path = /; exec.prestop = 'echo STOP'; persist; }" > jail.conf
+
+	atf_check -o ignore jail -f jail.conf -c "$jid"
+	atf_check -o match:"STOP" jail -f jail.conf -r "$jid"
+
+	# Ditto, but now we set a name for that jid-jail.
+	echo "$jid {name = basejail; path = /; exec.prestop = 'echo STOP'; persist; }" > jail.conf
+
+	atf_check -o ignore jail -f jail.conf -c "$jid"
+	atf_check -o match:"STOP" jail -f jail.conf -r "$jid"
+
+	# Confirm that we have a valid jid available in exec.poststop.  It's
+	# probably debatable whether we should or not.
+	echo "basejail {path = /; exec.poststop = 'echo JID=\$JID'; persist; }" > jail.conf
+	atf_check -o ignore jail -f jail.conf -c basejail
+	jid=$(jls -j basejail jid)
+
+	atf_check -o match:"JID=$jid" jail -f jail.conf -r basejail
+
+}
+
+param_consistency_cleanup()
+{
+	clean_jails
+
+	if [ -f "interfaces.lst" ]; then
+		while read iface; do
+			ifconfig "$iface" destroy
+		done < interfaces.lst
+	fi
+}
 
 atf_init_test_cases()
 {
@@ -172,4 +299,6 @@ atf_init_test_cases()
 	atf_add_test_case "list"
 	atf_add_test_case "nested"
 	atf_add_test_case "commands"
+	atf_add_test_case "jid_name_set"
+	atf_add_test_case "param_consistency"
 }