The branch main has been updated by kevans:
URL:
https://cgit.FreeBSD.org/src/commit/?id=02944d8c4969ffe97fcf84cb2ccb672e828c1d04
commit 02944d8c4969ffe97fcf84cb2ccb672e828c1d04
Author: Kyle Evans <kev...@freebsd.org>
AuthorDate: 2025-07-26 03:13:44 +0000
Commit: Kyle Evans <kev...@freebsd.org>
CommitDate: 2025-07-26 03:13:44 +0000
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: d8f021add40c3 ("jail: add JID, JNAME and JPATH to env [...]")
Reviewed by: jamie
Differential Revision: https://reviews.freebsd.org/D51502
---
usr.sbin/jail/command.c | 38 ++++++----
usr.sbin/jail/config.c | 13 +++-
usr.sbin/jail/jail.c | 7 ++
usr.sbin/jail/tests/jail_basic_test.sh | 129 +++++++++++++++++++++++++++++++++
4 files changed, 173 insertions(+), 14 deletions(-)
diff --git a/usr.sbin/jail/command.c b/usr.sbin/jail/command.c
index 8ea3f3ee8795..9da4fe51673a 100644
--- a/usr.sbin/jail/command.c
+++ b/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);
}
diff --git a/usr.sbin/jail/config.c b/usr.sbin/jail/config.c
index 3af0088626c9..70de82e662e7 100644
--- a/usr.sbin/jail/config.c
+++ b/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) {
diff --git a/usr.sbin/jail/jail.c b/usr.sbin/jail/jail.c
index 27769cc14958..46cabf76ae11 100644
--- a/usr.sbin/jail/jail.c
+++ b/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
diff --git a/usr.sbin/jail/tests/jail_basic_test.sh
b/usr.sbin/jail/tests/jail_basic_test.sh
index f3c8be4a6595..509900e8569c 100755
--- a/usr.sbin/jail/tests/jail_basic_test.sh
+++ b/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"
}
