Download raw body.
Maybe strlen is unnecessary in kern_unveil.c
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