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