simplify path handling in sysctl_try_reclaim_vnode
ClosedPublic
Actions

Authored by emaste on Oct 2 2019, 1:35 PM.

Details

Reviewers

kib
asomers

Commits

rS353021: simplify path handling in sysctl_try_reclaim_vnode

Summary

MAXPATHLEN / PATH_MAX includes space for the terminating NUL, and namei verifies the presence of the NUL. Thus there is no need to increase the buffer size here. Still write the NUL in case of misuse of the sysctl.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

emaste created this revision.Oct 2 2019, 1:35 PM

emaste mentioned this in D20519: Add a debugging facility to manually reclaim a vnode.

emaste added inline comments.Oct 2 2019, 1:41 PM

sys/kern/vfs_subr.c
358	presumably we could use `sysctl_handle_string` here instead, something like buf = malloc(PATH_MAX, M_TEMP, M_WAITOK); buf[0] = '\0'; error = sysctl_handle_string(oidp, arg1, arg2, req); if (error != 0) goto out; ndflags = ...

@kib pointed out on IRC that the original PATH_MAX + 1 is incorrect but arguably innocent and I agree, but think it is worth the change to avoid confusing folks (who find that code) into wondering if PATH_MAX includes space for the NUL or not - that's what prompted me to look at this in the first place.

emaste added a subscriber: pjd.Oct 2 2019, 1:43 PM

If PATH_MAX + 1 is a problem here, then it's also a problem at fs/nfsserver/nfs_nfsdport.c:{3520, 3527, 3534, 3542}.

Ah, I probably just grepped in sys/kern. I'll take a look at a patch for nfs_nfsdport.c in a bit - it's not so straightforward.

kib added inline comments.Oct 2 2019, 2:09 PM

sys/kern/vfs_subr.c
362	I am not sure that this is correct. If sysctl(8) sets newlen to strlen(), then -1 should not be done.

emaste added inline comments.Oct 2 2019, 2:29 PM

sys/kern/vfs_subr.c
362	mmm, how about `buf[min(req->newlen, PATH_MAX - 1)] = '\0';`

kib added inline comments.Oct 2 2019, 2:56 PM

sys/kern/vfs_subr.c
362	This should work.

Correct per @kib

kib added inline comments.Oct 2 2019, 4:44 PM

sys/kern/vfs_subr.c
354	This line also should be fixed. Or rather, I think part of the confusion is whether the terminating nul is counted for the newlen.

emaste added inline comments.Oct 2 2019, 4:55 PM

sys/kern/vfs_subr.c
354	IMO it's a separate issue of similar confusion. First issue is whether a buffer of `PATH_MAX` bytes is sufficient to hold a longest-possible pathname; it is. Second is whether strings passed to `sysctl` include the NUL in `newlen`, inspection shows they don't, so this could be `if (req->newlen >= PATH_MAX)`