[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJgzZopcoLSWrPu=yt6WXjX+cJ2CAth9R-=4NSMWrmv0=PhtOg@mail.gmail.com>
Date: Thu, 17 Feb 2022 16:29:16 -0800
From: enh <enh@...gle.com>
To: James Y Knight <jyknight@...gle.com>
Cc: libc-coord@...ts.openwall.com, Florian Weimer <fweimer@...hat.com>
Subject: Re: posix_spawn() support for close_range(CLOSE_RANGE_CLOEXEC)
On Mon, Jan 24, 2022 at 4:28 PM James Y Knight <jyknight@...gle.com> wrote:
>
>
> On Mon, Jan 24, 2022 at 5:47 PM Maciej Żenczykowski <maze@...gle.com>
> wrote:
>
>> On Mon, Jan 24, 2022 at 1:24 PM Florian Weimer <fweimer@...hat.com>
>> wrote:
>> >
>> > > we have a proposal to add a new posix_spawn() flag to bionic
>> > > (Android's libc) that would mark all file descriptors not otherwise
>> > > mentioned as close-on-exec.
>> > >
>> > > (see
>> https://android-review.googlesource.com/c/platform/bionic/+/1955269
>> > > for the proposed implementation.)
>> > >
>> > > anyone aware of any other work in this kind of direction?
>> >
>> > Solaris and glibc have posix_spawn_file_actions_addclosefrom_np (Solaris
>> > was first). Given that the execve is unavoidable (except for errors),
>> > that seems pretty much equivalent. Perhaps POSIX_SPAWN_CLOEXEC_DEFAULT
>> > is a little bit easier to use?
>>
>> Right, I realized that basically what I want/need is to close
>> everything except for a specific set of fds.
>> That set is stdin/out/err (closing these before exec is just a really
>> bad idea, they should at the minimum be /dev/null) and any file
>> descriptors I'm explicitly trying to pass (which requires explicit
>> posix_spawn_actions_adddup2 (maybe addopen) calls or it's racy wrt
>> other threads, since if they're opened pre-posix_spawn, then they
>> need/should be opened with O_CLOEXEC anyway, and we don't have a
>> posix_spawn_actions_adduncloexec helper besides dup2 - note that
>> dup2(X, X) doesn't work to unset O_CLOEXEC).
>
>
> I'm unclear what you mean to say here?
>
> Per <https://sourceware.org/bugzilla/show_bug.cgi?id=23640> and <
> https://www.austingroupbugs.net/view.php?id=411>
> posix_spawn_actions_adddup2 _should_ cause O_CLOEXEC to be cleared,
> although "dup2" itself does not.
>
> I don't know if bionic implements that, but if it doesn't, your proposed
> flag is going to be broken -- it will result in the broken behavior where
> the FD gets closed (if the process _did_ mark it CLOEXEC initially, and the
> input fd number happens to be the same in the parent, as the number you
> want to pass on to the subprocess).
>
d'oh! yeah, we'd missed this special case for
posix_spawn_file_actions_adddup2(). fixed by
https://android-review.googlesource.com/c/platform/bionic/+/1989308 in AOSP
for Android 13. thanks for pointing this out!
> adddup2 is also better than addopen, because it allows for better
>> error handling (dup2 is unlikely to fail, open much more so, logging
>> prior to posix_spawn() is easier).
>>
>> Hence, this approach seems to be by far the easiest to use.
>> Just set the flag, and your code works and doesn't leak fds any more
>> even if other threads are forgetting to set O_CLOEXEC and/or racing
>> with you.
>>
>
Content of type "text/html" skipped
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
