Re: [musl] fcntl: Purpose of second DUPFD_CLOEXEC?

[Rich Felker <dalias@libc.org>]

On Thu, Aug 22, 2024 at 03:52:58PM +0200, Markus Wichmann wrote:
> Hi all,
> 
> I just stumbled upon the code to emulate F_DUPFD_CLOEXEC on old kernels.
> At the moment, it looks like this:
> 
> |int ret = __syscall(SYS_fcntl, fd, F_DUPFD_CLOEXEC, arg);
> |if (ret != -EINVAL) {
> |	if (ret >= 0)
> |		__syscall(SYS_fcntl, ret, F_SETFD, FD_CLOEXEC);
> |	return __syscall_ret(ret);
> |}
> |ret = __syscall(SYS_fcntl, fd, F_DUPFD_CLOEXEC, 0);
> |if (ret != -EINVAL) {
> |	if (ret >= 0) __syscall(SYS_close, ret);
> |	return __syscall_ret(-EINVAL);
> |}
> |ret = __syscall(SYS_fcntl, fd, F_DUPFD, arg);
> |if (ret >= 0) __syscall(SYS_fcntl, ret, F_SETFD, FD_CLOEXEC);
> |return __syscall_ret(ret);
> 
> And I'm wondering what the point of the second call to F_DUPFD_CLOEXEC
> is. If it is just to test that the argument is valid, then why not just
> get rid of the block?

It's to test if F_DUPFD_CLOEXEC failed with EINVAL because it's an
unknown fcntl command or because arg exceeds the fd limit.
Unfortunately, the error is ambiguous.

> F_DUPFD has the same constraints on the argument.

That's a good point -- F_DUPFD can be expected to fail (thus not
introduce any fd leak race condition) as long as the limit does not
change concurrently with the fcntl call. But if the limit does change
concurrently, the test result may be outdated by the time it's used.
So I think it may make sense to try the fallback directly.

> Getting rid of that block also has the advantage of being able to factor
> out the F_SETFD call. Then it would simply be
> 
> int ret = __syscall(SYS_fcntl, fd, F_DUPFD_CLOEXEC, arg);
> if (ret == -EINVAL) ret = __syscall(SYS_fcntl, fd, F_DUPFD, arg);
> if (ret >= 0) __syscall(SYS_fcntl, fd, F_SETFL, FD_CLOEXEC);
> return __syscall_ret(ret);
> 
> Much nicer, isn't it? And it doesn't even allocate a file descriptor
> uselessly.

Seems like it. I'll wait for further feedback but this might be the
right thing to do.

Note that this factoring-out by inverting the conditions can be done
even without dropping the arg=0 second check.

Rich