Download raw body.
Maybe strlen is unnecessary in kern_unveil.c
like ah. no?
the point of it being named "size" and not "len" is to make it clear it is the size of the allocation and not the length of a string.
renaming it to len makes it confusing, which is only partly mitigated by a comment. this is not a helpful visit to the bikeshed paint store
> On Nov 19, 2025, at 06:10, Sebastien Marie <semarie@kapouay.eu.org> wrote:
>
> Christian Schulte <cs@schulte.it> writes:
>
>> Stumbled upon this while reading. Not sure about the internals of
>> struct componentname in namei.h, but I think kern_unveil.c does not
>> need to call strlen. Note that cn_namelen in struct componentname
>> is of type long and namesize in struct unvname is of type size_t.
>> Not sure this is an issue.
>
> Thanks for your diff about strlen and kern_unveil.c.
>
> It makes me reread the whole thing and I agree that strlen() isn't
> necessary.
>
> But I also have been triggered by +1 and -1 in the code for manipulating
> a string.
>
> In the current code, unvname_new() function assumes `char *name` to be a
> string (NUL terminated). Same for unveil_add_name_unlocked() where
> strlen() is used on `char *name`.
>
> On the order hand, unveil_add() is calling unveil_namelookup() with
> `cn_nameptr` which is *not* a NUL terminated string in the general case
> (I think it could be NUL terminated only when considering the terminal
> component).
>
> To unify the usage of `cn_nameptr`, I would like to change the struct
> unvname to the following:
>
> struct unvname {
> - char *un_name;
> - size_t un_namesize;
> + char *un_name; /* without terminal '\0' */
> + size_t un_namelen;
> u_char un_flags;
> RBT_ENTRY(unvnmae) un_rbt;
> };
>
> it renames un_namesize field to un_namelen and documents that un_name is
> without terminal NUL char.
>
> the rest of code changes the places where NUL terminated string was
> assumed (mostly DPRINTF), and reuse your initial diff for passing
> cn_namelen alongside cn_nameptr.
>
> I left size_t for un_namelen unchanged.
>
> regress/sys/kern/unveil is still passing.
>
> Comments or OK ?
> --
> Sebastien Marie
>
> diff --git a/sys/kern/kern_unveil.c b/sys/kern/kern_unveil.c
> index 492269e48a..b37bc217e7 100644
> --- a/sys/kern/kern_unveil.c
> +++ b/sys/kern/kern_unveil.c
> @@ -36,8 +36,8 @@
> #include <sys/pledge.h>
>
> struct unvname {
> - char *un_name;
> - size_t un_namesize;
> + char *un_name; /* without terminal '\0' */
> + size_t un_namelen;
> u_char un_flags;
> RBT_ENTRY(unvnmae) un_rbt;
> };
> @@ -65,19 +65,19 @@
> static inline int
> unvname_compare(const struct unvname *n1, const struct unvname *n2)
> {
> - if (n1->un_namesize == n2->un_namesize)
> - return (memcmp(n1->un_name, n2->un_name, n1->un_namesize));
> + if (n1->un_namelen == n2->un_namelen)
> + return (memcmp(n1->un_name, n2->un_name, n1->un_namelen));
> else
> - return (n1->un_namesize - n2->un_namesize);
> + return (n1->un_namelen - n2->un_namelen);
> }
>
> struct unvname *
> -unvname_new(const char *name, size_t size, u_char flags)
> +unvname_new(const char *name, size_t namelen, u_char flags)
> {
> struct unvname *ret = malloc(sizeof(struct unvname), M_PROC, M_WAITOK);
> - ret->un_name = malloc(size, M_PROC, M_WAITOK);
> - memcpy(ret->un_name, name, size);
> - ret->un_namesize = size;
> + ret->un_name = malloc(namelen, M_PROC, M_WAITOK);
> + memcpy(ret->un_name, name, namelen);
> + ret->un_namelen = namelen;
> ret->un_flags = flags;
> return ret;
> }
> @@ -85,7 +85,7 @@
> void
> unvname_delete(struct unvname *name)
> {
> - free(name->un_name, M_PROC, name->un_namesize);
> + free(name->un_name, M_PROC, name->un_namelen);
> free(name, M_PROC, sizeof(struct unvname));
> }
>
> @@ -111,54 +111,55 @@
> }
>
> int
> -unveil_add_name_unlocked(struct unveil *uv, char *name, u_char flags)
> +unveil_add_name_unlocked(struct unveil *uv, char *name, size_t namelen, u_char flags)
> {
> struct unvname *unvn;
>
> - unvn = unvname_new(name, strlen(name) + 1, flags);
> + unvn = unvname_new(name, namelen, flags);
> if (RBT_INSERT(unvname_rbt, &uv->uv_names, unvn) != NULL) {
> /* Name already present. */
> unvname_delete(unvn);
> return 0;
> }
>
> - DPRINTF("added name %s underneath vnode %p\n", name, uv->uv_vp);
> + DPRINTF("added name %.*s underneath vnode %p\n", (int)namelen, name,
> + uv->uv_vp);
> return 1;
> }
>
> int
> -unveil_add_name(struct unveil *uv, char *name, u_char flags)
> +unveil_add_name(struct unveil *uv, char *name, size_t namelen, u_char flags)
> {
> int ret;
>
> rw_enter_write(&uv->uv_lock);
> - ret = unveil_add_name_unlocked(uv, name, flags);
> + ret = unveil_add_name_unlocked(uv, name, namelen, flags);
> rw_exit_write(&uv->uv_lock);
> return ret;
> }
>
> struct unvname *
> -unveil_namelookup(struct unveil *uv, char *name)
> +unveil_namelookup(struct unveil *uv, char *name, size_t namelen)
> {
> struct unvname n, *ret = NULL;
>
> rw_enter_read(&uv->uv_lock);
>
> - DPRINTF("%s: looking up name %s (%p) in vnode %p\n",
> - __func__, name, name, uv->uv_vp);
> + DPRINTF("%s: looking up name %.*s (%p) in vnode %p\n",
> + __func__, (int)namelen, name, name, uv->uv_vp);
>
> KASSERT(uv->uv_vp != NULL);
>
> n.un_name = name;
> - n.un_namesize = strlen(name) + 1;
> + n.un_namelen = namelen;
>
> ret = RBT_FIND(unvname_rbt, &uv->uv_names, &n);
>
> rw_exit_read(&uv->uv_lock);
>
> - DPRINTF("%s: %s name %s in vnode %p\n", __func__,
> + DPRINTF("%s: %s name %.*s in vnode %p\n", __func__,
> (ret == NULL) ? "no match for" : "matched",
> - name, uv->uv_vp);
> + (int)namelen, name, uv->uv_vp);
> return ret;
> }
>
> @@ -221,7 +222,8 @@
> rw_enter_read(&from->uv_lock);
> RBT_FOREACH_SAFE(unvn, unvname_rbt, &from->uv_names, next) {
> if (unveil_add_name_unlocked(&child->ps_uvpaths[i],
> - unvn->un_name, unvn->un_flags))
> + unvn->un_name, unvn->un_namelen,
> + unvn->un_flags))
> child->ps_uvncount++;
> }
> rw_exit_read(&from->uv_lock);
> @@ -473,10 +475,12 @@
> if (!directory_add) {
> struct unvname *tname;
> if ((tname = unveil_namelookup(uv,
> - ndp->ni_cnd.cn_nameptr)) != NULL) {
> - DPRINTF("unveil: %s(%d): changing flags for %s"
> + ndp->ni_cnd.cn_nameptr,
> + ndp->ni_cnd.cn_namelen)) != NULL) {
> + DPRINTF("unveil: %s(%d): changing flags for %.*s"
> "in vnode %p, uvcount %d\n",
> - pr->ps_comm, pr->ps_pid, tname->un_name, vp,
> + pr->ps_comm, pr->ps_pid,
> + (int)tname->un_namelen, tname->un_name, vp,
> vp->v_uvcount);
>
> if (!unveil_setflags(&tname->un_flags, flags))
> @@ -511,13 +515,15 @@
> goto done;
> }
>
> - if (unveil_add_name(uv, ndp->ni_cnd.cn_nameptr, flags))
> + if (unveil_add_name(uv, ndp->ni_cnd.cn_nameptr, ndp->ni_cnd.cn_namelen,
> + flags))
> pr->ps_uvncount++;
> ret = 0;
>
> - DPRINTF("unveil: %s(%d): added name %s beneath %s vnode %p,"
> + DPRINTF("unveil: %s(%d): added name %.*s beneath %s vnode %p,"
> " uvcount %d\n",
> - pr->ps_comm, pr->ps_pid, ndp->ni_cnd.cn_nameptr,
> + pr->ps_comm, pr->ps_pid,
> + (int)ndp->ni_cnd.cn_namelen, ndp->ni_cnd.cn_nameptr,
> uv->uv_flags ? "unrestricted" : "restricted",
> vp, vp->v_uvcount);
>
> @@ -701,9 +707,9 @@
>
> }
> /* directory and flags match, success */
> - DPRINTF("unveil: %s(%d): matched directory \"%s\" at vnode %p\n",
> - pr->ps_comm, pr->ps_pid, ni->ni_cnd.cn_nameptr,
> - uv->uv_vp);
> + DPRINTF("unveil: %s(%d): matched directory \"%.*s\" at vnode %p\n",
> + pr->ps_comm, pr->ps_pid, (int)ni->ni_cnd.cn_namelen,
> + ni->ni_cnd.cn_nameptr, uv->uv_vp);
>
> return (0);
> }
> @@ -716,18 +722,19 @@
>
> goto done;
> }
> - if ((tname = unveil_namelookup(uv, ni->ni_cnd.cn_nameptr)) == NULL) {
> - DPRINTF("unveil: %s(%d) no match for terminal '%s' in "
> + if ((tname = unveil_namelookup(uv, ni->ni_cnd.cn_nameptr,
> + ni->ni_cnd.cn_namelen)) == NULL) {
> + DPRINTF("unveil: %s(%d) no match for terminal '%.*s' in "
> "directory vnode %p\n",
> - pr->ps_comm, pr->ps_pid,
> + pr->ps_comm, pr->ps_pid, (int)ni->ni_cnd.cn_namelen,
> ni->ni_cnd.cn_nameptr, ni->ni_dvp);
>
> /* no specific name, so check unveil directory flags */
> if (!unveil_flagmatch(ni, uv->uv_flags)) {
> DPRINTF("unveil: %s(%d) terminal "
> - "'%s' flags mismatch in directory "
> + "'%.*s' flags mismatch in directory "
> "vnode %p\n",
> - pr->ps_comm, pr->ps_pid,
> + pr->ps_comm, pr->ps_pid, (int)ni->ni_cnd.cn_namelen,
> ni->ni_cnd.cn_nameptr, ni->ni_dvp);
>
> /*
> @@ -747,23 +754,23 @@
> goto done;
> }
> /* directory flags match, success */
> - DPRINTF("unveil: %s(%d): matched \"%s\" underneath vnode %p\n",
> - pr->ps_comm, pr->ps_pid, ni->ni_cnd.cn_nameptr,
> - uv->uv_vp);
> + DPRINTF("unveil: %s(%d): matched \"%.*s\" underneath vnode %p\n",
> + pr->ps_comm, pr->ps_pid, (int)ni->ni_cnd.cn_namelen,
> + ni->ni_cnd.cn_nameptr, uv->uv_vp);
>
> return (0);
> }
> if (!unveil_flagmatch(ni, tname->un_flags)) {
> /* do flags match for matched name */
> - DPRINTF("unveil: %s(%d) flag mismatch for terminal '%s'\n",
> - pr->ps_comm, pr->ps_pid, tname->un_name);
> + DPRINTF("unveil: %s(%d) flag mismatch for terminal '%.*s'\n",
> + pr->ps_comm, pr->ps_pid, (int)tname->un_namelen, tname->un_name);
>
> pr->ps_acflag |= AUNVEIL;
> return EACCES;
> }
> /* name and flags match. success */
> - DPRINTF("unveil: %s(%d) matched terminal '%s'\n",
> - pr->ps_comm, pr->ps_pid, tname->un_name);
> + DPRINTF("unveil: %s(%d) matched terminal '%.*s'\n",
> + pr->ps_comm, pr->ps_pid, (int)tname->un_namelen, tname->un_name);
>
> return (0);
>
> @@ -774,9 +781,10 @@
> */
> for (uv = ni->ni_unveil_match; uv != NULL; uv = nuv) {
> if (unveil_flagmatch(ni, uv->uv_flags)) {
> - DPRINTF("unveil: %s(%d): matched \"%s\" underneath/at "
> + DPRINTF("unveil: %s(%d): matched \"%.*s\" underneath/at "
> "vnode %p\n", pr->ps_comm, pr->ps_pid,
> - ni->ni_cnd.cn_nameptr, uv->uv_vp);
> + (int)ni->ni_cnd.cn_namelen, ni->ni_cnd.cn_nameptr,
> + uv->uv_vp);
>
> return (0);
> }
Maybe strlen is unnecessary in kern_unveil.c