simplify path handling in sysctl_try_reclaim_vnode

[emaste]

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.

[emaste]

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]

@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.

[asomers]

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]

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]

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]

sys/kern/vfs_subr.c

361 ↗

(On Diff #62830)

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

[kib]

sys/kern/vfs_subr.c

361 ↗

(On Diff #62830)

This should work.

[emaste]

Correct per @kib

[kib]

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]

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]

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.