Page MenuHomeFreeBSD

Improve error handling for KERN_PROC_{CWD,FILEDESC}.
ClosedPublic

Authored by markj on May 29 2018, 6:13 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jan 7, 8:37 PM
Unknown Object (File)
Dec 22 2022, 5:12 AM
Unknown Object (File)
Dec 13 2022, 10:02 PM
Unknown Object (File)
Dec 5 2022, 7:27 AM
Unknown Object (File)
Nov 27 2022, 9:59 AM

Details

Summary

We were returning 0 in places where the output was being truncated. I
couldn't find any consumers of KERN_PROC_CWD, but kinfo_getfile(3)
pretty clearly shouldn't be returning a truncated file array.

This change is incomplete, as most callers of export_vnode_to_sb() do
not check the return value and thus don't detect truncation. It's
sufficient to fix KERN_PROC_CWD though.

Test Plan

Will test tmux, as suggested by mjg@.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I have no opinion one way or the other.

tmux uses the feature and should be verified.

markj added reviewers: cem, jhb.

I don't see how this affects tmux in any way, unless the error code breaks the previously working-but-truncated path(s). If that's the case, I think those should be changed to swallow ENOMEM. But then, what's the point of propagating the error up?

sys/kern/kern_descrip.c
4297–4300

Is the ordering of sbuf_bcat() -> sbuf_error() well-defined as used here? Or should this be an if()/else?

In D15607#329598, @cem wrote:

I don't see how this affects tmux in any way, unless the error code breaks the previously working-but-truncated path(s). If that's the case, I think those should be changed to swallow ENOMEM. But then, what's the point of propagating the error up?

Looking at the tmux code, I don't believe this change will have an effect. It would just be a sanity test.

sys/kern/kern_descrip.c
4297–4300

The "?" operator is a sequence point, i.e., the first expression must be evaluated before the ones following "?".

sys/kern/kern_descrip.c
4297–4300

I think I'd still prefer if/else for clarity. It doesn't fit in a single line anymore anyways.

markj added inline comments.
sys/kern/kern_descrip.c
4297–4300

Ok. It didn't fit before either (> 80 cols).

Looks fine to me, with the caveat that I haven't looked at what might break with an error return. I'm most interested in the impact on core dumps.

sys/kern/kern_descrip.c
4297–4300

Heh, oops :-)

This revision is now accepted and ready to land.May 29 2018, 8:39 PM

I do think it was intentional to truncate once the output buffer was exhausted rather than failing with ENOMEM. Its debatable if that is a useful thing or not. Most sysctls return ENOMEM instead so that the request can be retried with a larger output buffer so it is probably better to do what this does.

In D15607#330347, @jhb wrote:

I do think it was intentional to truncate once the output buffer was exhausted rather than failing with ENOMEM. Its debatable if that is a useful thing or not. Most sysctls return ENOMEM instead so that the request can be retried with a larger output buffer so it is probably better to do what this does.

That makes sense in sysctl context. I'm not sure it makes sense in core dump context. Maybe it does, but it would probably require additional changes there.

In D15607#330348, @cem wrote:
In D15607#330347, @jhb wrote:

I do think it was intentional to truncate once the output buffer was exhausted rather than failing with ENOMEM. Its debatable if that is a useful thing or not. Most sysctls return ENOMEM instead so that the request can be retried with a larger output buffer so it is probably better to do what this does.

That makes sense in sysctl context. I'm not sure it makes sense in core dump context. Maybe it does, but it would probably require additional changes there.

Exactly, and the error should be propagated up so that the right layer (e.g., sysctl vs. core dump) can decide whether to throw it away or not. As it happens, the core dump code already ignores these errors, so this change is for the benefit of sysctl interfaces.

Exactly, and the error should be propagated up so that the right layer (e.g., sysctl vs. core dump) can decide whether to throw it away or not. As it happens, the core dump code already ignores these errors, so this change is for the benefit of sysctl interfaces.

Ah, perfect.

I think so. I'm doing some more testing and will commit it later today (with error checking added to export_vnode_to_sb() callers).