From: Bob Beck Subject: Re: Maybe strlen is unnecessary in kern_unveil.c To: Sebastien Marie Cc: Christian Schulte , tech@openbsd.org Date: Wed, 19 Nov 2025 08:11:21 -0700 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 wrote: > > Christian Schulte 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 > > 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); > }