Bug 23640 - no way to easily clear FD_CLOEXEC in posix_spawn_file_actions_adddup2()
Summary: no way to easily clear FD_CLOEXEC in posix_spawn_file_actions_adddup2()
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: 2.29
: P2 normal
Target Milestone: 2.29
Assignee: Adhemerval Zanella
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-09-12 15:53 UTC by Eric Blake
Modified: 2019-01-03 16:50 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Blake 2018-09-12 15:53:08 UTC
http://austingroupbugs.net/view.php?id=411 proposes adding more atomic FD_CLOEXEC into POSIX interfaces, in part based on what glibc has done. But it picked up on one thing that glibc has not yet changed: if you have a multi-threaded app that has been converted to properly use O_CLOEXEC on ALL file opens, and then want to hand-choose which fds are then purposefully inherited into a child process, your normal course of action is to use fcntl(F_SETFD) in between fork() and exec*() - but there is no counterpart to this when using posix_spawn() instead of fork/exec.  Unfortunately, using posix_spawn_file_actions_adddup2(&act, fd, fd) is currently a no-op in glibc - if fd already has FD_CLOEXEC set, the child process will not inherit fd.  A smarter course of action is to have the dup2 action in the file_actions list result in the proper fcntl() in the child process, as a way of intentionally marking an fd as inherited into that particular child while still remaining FD_CLOEXEC in the parent.

Here's what the POSIX wording has to say:

"Also, I don't know of any platform where posix_spawn_file_actions_adddup2( )
can clear FD_CLOEXEC, but that seems like a useful change to make while
tightening the specification on how to properly use FD_CLOEXEC, since the
only standardized alternative for explicitly handing a file descriptor to
the child process while leaving it FD_CLOEXEC in the parent process involves
more complexity and risks EMFILE failure.
...


At line 46976 [XSH posix_spawn_file_actions_adddup2], add a sentence:

If fildes and newfildes are equal, then the action shall ensure that
the FD_CLOEXEC flag of fildes is cleared (even though dup2( ) would
leave it unchanged).

After line 46999 [XSH posix_spawn_file_actions_adddup2], add the
following:

Although dup2( ) is required to do nothing when fildes and newfildes
are equal and fildes is an open descriptor, the use of
posix_spawn_file_actions_adddup2( ) is required to clear the
FD_CLOEXEC flag of fildes. This is because there is no counterpart of
posix_spawn_file_actions_fcntl( ) that could be used for clearing the
flag; it would also be possible to achieve this effect by using two
calls to posix_spawn_file_actions_adddup2( ) and a temporary fildes
value known to not conflict with any other file descriptors, coupled
with a posix_spawn_file_actions_close( ) to avoid leaking the
temporary, but this approach is complex, and risks EMFILE or ENFILE
failure that can be avoided with the in-place removal of FD_CLOEXEC.

There is no need for posix_spawn_file_actions_adddup3( ), since it
makes no sense to create a file descriptor with FD_CLOEXEC set before
spawning the child process, where that file descriptor would
immediately be closed again.
Comment 1 Eric Blake 2018-09-12 16:10:08 UTC
The attachment to POSIX bug 411 at http://austingroupbugs.net/file_download.php?file_id=36&type=bug has a slightly updated wording proposal:

At page 1433 line 46976 [XSH posix_spawn_file_actions_adddup2( ) DESCRIPTION], add a sentence:
If fildes and newfildes are equal, then the action shall ensure that newfildes is inherited by the new process with FD_CLOEXEC clear, even if the FD_CLOEXEC flag of fildes is set at the time the new process is spawned, and even though dup2( ) would not make such a change.
Comment 2 Adhemerval Zanella 2018-09-19 15:33:00 UTC
I think the rationale to have a posix_spawn operation to reset O_CLOEXEC is reasonable, but I am not very found of having dup2 and posix_spawn_file_actions_adddup2 with subtle different semantics: this usually lead to wrong assumptions, since posix_spawn file actions are in fact modeled based on POSIX counterparts.

I am not sure which would be the best extension to add though (maybe posix_spawn_file_actions_fcntl?).
Comment 3 Eric Blake 2018-09-19 16:53:01 UTC
(In reply to Adhemerval Zanella from comment #2)
> I think the rationale to have a posix_spawn operation to reset O_CLOEXEC is
> reasonable, but I am not very found of having dup2 and
> posix_spawn_file_actions_adddup2 with subtle different semantics: this
> usually lead to wrong assumptions, since posix_spawn file actions are in
> fact modeled based on POSIX counterparts.
> 
> I am not sure which would be the best extension to add though (maybe
> posix_spawn_file_actions_fcntl?).

Adding posix_spawn_file_actions_addfcntl seems like a lot of complexity. In particular, the fact that you have to read-modify-write in order to properly use fcntl() makes it rather verbose to have to add multiple fcntl actions to file_actions, compared to having a single call just give the semantics you want.  Another drawback of adding an _addfcntl is deciding what things it supports, as well as documenting the vast majority of things it specifically does not support (updating file locking, for example, seems like a huge growth in scope and unlikely to ever be natively part of a spawn() syscall, but having a spawn syscall that can clear FD_CLOEXEC on fds specifically requested for use in the child does not seem difficult).

A posix_spawn_file_actions_adddup3() doesn't make much sense either - the normal dup3() is specifically documented to require failure on dup3(fd, fd, flags) (unlike dup2(fd, fd) which must be a no-op).

You could go with an even more simplistic approach, and add posix_spawn_file_actions_addnocloexec(&act, fd) (instead of _adddup2(&act, fd, fd)), where the different function name has no direct bare counterpart, but makes it immediately obvious that its goal is to remove FD_CLOEXEC flags on fds that must remain cloexec in the parent, but must be inherited by the child.

Of course, if glibc actually implements something different than the current POSIX proposal, it's not too late to use that as feedback to update the POSIX proposal to match existing practice (after all, the proposal is still just that, and hasn't actually been incorporated into a released version of POSIX yet).  I'm just pointing out that when this proposal was first vetted by the Austin Group (several years ago now), no one had any better ideas for an easy way to clear FD_CLOEXEC, besides making _adddup2() have a documented slightly different behavior from normal dup2().
Comment 4 Adhemerval Zanella 2018-09-19 17:03:15 UTC
(In reply to Eric Blake from comment #3)
> (In reply to Adhemerval Zanella from comment #2)
> > I think the rationale to have a posix_spawn operation to reset O_CLOEXEC is
> > reasonable, but I am not very found of having dup2 and
> > posix_spawn_file_actions_adddup2 with subtle different semantics: this
> > usually lead to wrong assumptions, since posix_spawn file actions are in
> > fact modeled based on POSIX counterparts.
> > 
> > I am not sure which would be the best extension to add though (maybe
> > posix_spawn_file_actions_fcntl?).
> 
> Adding posix_spawn_file_actions_addfcntl seems like a lot of complexity. In
> particular, the fact that you have to read-modify-write in order to properly
> use fcntl() makes it rather verbose to have to add multiple fcntl actions to
> file_actions, compared to having a single call just give the semantics you
> want.  Another drawback of adding an _addfcntl is deciding what things it
> supports, as well as documenting the vast majority of things it specifically
> does not support (updating file locking, for example, seems like a huge
> growth in scope and unlikely to ever be natively part of a spawn() syscall,
> but having a spawn syscall that can clear FD_CLOEXEC on fds specifically
> requested for use in the child does not seem difficult).

I agree addfcntl would add a lot of complexity and we would need to add extensive documentation it is only intended to provide a subset of default fcntl operations (which I also think it is not a good idea).

> 
> A posix_spawn_file_actions_adddup3() doesn't make much sense either - the
> normal dup3() is specifically documented to require failure on dup3(fd, fd,
> flags) (unlike dup2(fd, fd) which must be a no-op).

Agreed.

> 
> You could go with an even more simplistic approach, and add
> posix_spawn_file_actions_addnocloexec(&act, fd) (instead of _adddup2(&act,
> fd, fd)), where the different function name has no direct bare counterpart,
> but makes it immediately obvious that its goal is to remove FD_CLOEXEC flags
> on fds that must remain cloexec in the parent, but must be inherited by the
> child.
> 
> Of course, if glibc actually implements something different than the current
> POSIX proposal, it's not too late to use that as feedback to update the
> POSIX proposal to match existing practice (after all, the proposal is still
> just that, and hasn't actually been incorporated into a released version of
> POSIX yet).  I'm just pointing out that when this proposal was first vetted
> by the Austin Group (several years ago now), no one had any better ideas for
> an easy way to clear FD_CLOEXEC, besides making _adddup2() have a documented
> slightly different behavior from normal dup2().

It looks like some libcs (musl for instance) are already pushing this proposal as expected behavior and I would like to avoid either adding a non-portable extension or deviating from POSIX.

I will work on adding this support on GLIBC, thanks to bring this up.
Comment 5 Sourceware Commits 2019-01-03 16:50:08 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  805334b26c7e6e83557234f2008497c72176a6cd (commit)
      from  03992356e6fedc5a5e9d32df96c1a2c79ea28a8f (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=805334b26c7e6e83557234f2008497c72176a6cd

commit 805334b26c7e6e83557234f2008497c72176a6cd
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date:   Wed Sep 19 12:14:34 2018 -0700

    posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)
    
    Austin Group issue #411 [1] proposes that posix_spawn file action
    posix_spawn_file_actions_adddup2 resets the close-on-exec when
    source and destination refer to same file descriptor.
    
    It solves the issue on multi-thread applications which uses
    close-on-exec as default, and want to hand-chose specifically
    file descriptor to purposefully inherited into a child process.
    Current approach to achieve this scenario is to use two adddup2 file
    actions and a temporary file description which do not conflict with
    any other, coupled with a close file action to avoid leaking the
    temporary file descriptor.  This approach, besides being complex,
    may fail with EMFILE/ENFILE file descriptor exaustion.
    
    This can be more easily accomplished with an in-place removal of
    FD_CLOEXEC.  Although the resulting adddup2 semantic is slight
    different than dup2 (equal file descriptors should be handled as
    no-op), the proposed possible solution are either more complex
    (fcntl action which a limited set of operations) or results in
    unrequired operations (dup3 which also returns EINVAL for same
    file descriptor).
    
    Checked on aarch64-linux-gnu.
    
    	[BZ #23640]
    	* posix/tst-spawn.c (do_prepare, handle_restart, do_test): Add
    	posix_spawn_file_actions_adddup2 test to check O_CLOCEXEC reset.
    	* sysdeps/unix/sysv/linux/spawni.c (__spawni_child): Add
    	close-on-exec reset for adddup2 file action.
    	* sysdeps/posix/spawni.c (__spawni_child): Likewise.
    
    [1] http://austingroupbugs.net/view.php?id=411

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                        |    9 +++++++
 posix/tst-spawn.c                |   44 +++++++++++++++++++++++++++++++-------
 sysdeps/posix/spawni.c           |   18 +++++++++++++--
 sysdeps/unix/sysv/linux/spawni.c |   18 +++++++++++++--
 4 files changed, 75 insertions(+), 14 deletions(-)
Comment 6 Adhemerval Zanella 2019-01-03 16:50:42 UTC
Fixed on 2.29 (commit 805334b26c).