Redefine fd_is_valid() to work with kern.trap_enotcap=1.

[markj]

Summary

fcntl() requires capability rights, whereas dup2() does not.

Also remove some redundant checks for valid fds, and add a test to
verify that nvlist_send()ing an nvlist with an invalid fd returns EBADF.
In particular, fcntl(F_DUPFD_CLOEXEC) will return EBADF if the fd is not
valid, and sendmsg() will return EBADF if asked to send a SCM_RIGHTS
message containing invalid descriptors. In some places
(nvlist_move_descriptor(), for example) we keep the check. I would as
much as possible like to reduce the number of system calls involved in
nvlist operations.

Test Plan

nvlist tests.

I have a capsicumized program that traps during startup when kern.trap_enotcap=1; with this change it starts without crashing.

[oshogbo]

I didn't check that, but I wonder what would happen if we would past the AT_FDCWD? Which have a negative value.

[markj]

In D17963#383743, @oshogbo wrote:

I didn't check that, but I wonder what would happen if we would past the AT_FDCWD? Which have a negative value.

You'd get EBADF, same as with fcntl(AT_FDCWD).

[oshogbo]

I would wait one day for @pjd

[cem]

lib/libnv/common_impl.h

38 ↗

(On Diff #50338)

Why don't we make this an inline function while we're here? Nothing about this construction requires it to be a macro, and the macro construction permits stupid misuses like fd_is_valid(n++) (which then has multiple surprising side effects).

(There's also always going to be a TOCTOU race between any fd_is_valid() and actual use, so I prefer to rely on the system calls which use the fd to check validity, but that's out of scope for this change.)

lib/libnv/tests/nvlist_send_recv_test.c

353 ↗

(On Diff #50338)

errno or error?

[markj]

lib/libnv/common_impl.h

38 ↗

(On Diff #50338)

Sure, I'll make it a function.

Indeed, I'm not convinced that we should do these checks at all. I didn't want to change the existing behaviour in this change though; right now, if you write nvlist_move_descriptor(nvl, "fd", -1);, then nvlist_error(nvl) will return a non-zero value as a result of this checking. Obviously there's nothing preventing one from closing the descriptor after inserting into the nvlist though.

lib/libnv/tests/nvlist_send_recv_test.c

353 ↗

(On Diff #50338)

nvlist_send() returns -1 and sets errno on error, so I think what's there now is right.

[cem]

LGTM modulo the inline function discussed earlier.

lib/libnv/common_impl.h

38 ↗

(On Diff #50338)

Thanks.

Yeah, I'm not sure I like the checking. On the one hand, it seems convenient for developers — catch mistakes sooner to when they happen. But as an API guarantee for the library to provide, it's obnoxious.

lib/libnv/tests/nvlist_send_recv_test.c

353 ↗

(On Diff #50338)

Ok, thanks.

[markj]

• Use fcntl(F_GETFD) instead of dup2(). It does not require capability rights either (I mistakenly assumed all fcntl() verbs did), and is marginally cheaper since it acquires the fd table's shared lock instead of the exclusive lock.
• Make fd_is_valid() a C function.

[cem]

lib/libnv/common_impl.h

40 ↗

(On Diff #50366)

bool please :)

[markj]

• Use bool.