Page MenuHomeFreeBSD

simplify path handling in sysctl_try_reclaim_vnode
ClosedPublic

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

Details

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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

emaste added inline comments.Wed, Oct 2, 1:41 PM
sys/kern/vfs_subr.c
358 ↗(On Diff #62830)

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 = ...
emaste added a comment.Wed, Oct 2, 1:43 PM

@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.Wed, Oct 2, 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}.

emaste added a comment.Wed, Oct 2, 1:52 PM

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.Wed, Oct 2, 2:09 PM
sys/kern/vfs_subr.c
361 ↗(On Diff #62830)

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.Wed, Oct 2, 2:29 PM
sys/kern/vfs_subr.c
361 ↗(On Diff #62830)

mmm, how about
buf[min(req->newlen, PATH_MAX - 1)] = '\0';

kib added inline comments.Wed, Oct 2, 2:56 PM
sys/kern/vfs_subr.c
361 ↗(On Diff #62830)

This should work.

emaste updated this revision to Diff 62836.Wed, Oct 2, 3:33 PM

Correct per @kib

kib added inline comments.Wed, Oct 2, 4:44 PM
sys/kern/vfs_subr.c
354 ↗(On Diff #62836)

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.Wed, Oct 2, 4:55 PM
sys/kern/vfs_subr.c
354 ↗(On Diff #62836)

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)

emaste updated this revision to Diff 62840.Wed, Oct 2, 5:00 PM

Drop min and make the test >= PATH_MAX. As the sysctl passes the string excluding the NUL req->newlen equal to PATH_MAX is too long.

kib accepted this revision.Wed, Oct 2, 5:10 PM
This revision is now accepted and ready to land.Wed, Oct 2, 5:10 PM
This revision was automatically updated to reflect the committed changes.