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

Authored by markj on May 29 2018, 6:13 PM.

Details

Reviewers

cem
jhb

Commits

rG36bd49ac4db8: fd: Avoid truncating output buffers for KERN_PROC_{CWD,FILEDESC}

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

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 16899
Build 16776: arc lint + arc unit

Event Timeline

markj created this revision.May 29 2018, 6:13 PM

Harbormaster completed remote builds in B16899: Diff 43104.May 29 2018, 6:13 PM

I have no opinion one way or the other.

tmux uses the feature and should be verified.

markj edited the test plan for this revision. (Show Details)May 29 2018, 7:42 PM

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
3515–3516	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
3515–3516	The "?" operator is a sequence point, i.e., the first expression must be evaluated before the ones following "?".

cem added inline comments.May 29 2018, 8:05 PM

sys/kern/kern_descrip.c
3515–3516	I think I'd still prefer if/else for clarity. It doesn't fit in a single line anymore anyways.

Use an if statement.

Harbormaster completed remote builds in B16901: Diff 43107.May 29 2018, 8:15 PM

markj marked an inline comment as done.May 29 2018, 8:16 PM

markj added inline comments.

sys/kern/kern_descrip.c
3515–3516	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
3515–3516	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.

In D15607#330352, @markj wrote:

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.

This came up again via: https://blog.guillaume-gomez.fr/articles/2021-12-14+sysinfo%3A+version+0.22+and+FreeBSD+support

Is it good to commit?

In D15607#756096, @allanjude wrote:

This came up again via: https://blog.guillaume-gomez.fr/articles/2021-12-14+sysinfo%3A+version+0.22+and+FreeBSD+support

Is it good to commit?

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

Closed by commit rG36bd49ac4db8: fd: Avoid truncating output buffers for KERN_PROC_{CWD,FILEDESC} (authored by markj). · Explain WhyDec 17 2021, 6:13 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rG36bd49ac4db8: fd: Avoid truncating output buffers for KERN_PROC_{CWD,FILEDESC}.

Herald added a subscriber: imp. · View Herald TranscriptDec 17 2021, 6:13 PM

emaste added a subscriber: emaste.Dec 17 2021, 6:28 PM