1456911 - LaunchApp doesn't clear close-on-exec when a file descriptor is mapped to itself

Status: RESOLVED FIXED

(Core :: IPC, defect, P2)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I haven't tested this yet, but it seems to be the case from code inspection: if a file descriptor in LaunchOptions::fds_to_remap has the same numeric value in the parent as the child, it won't have its close-on-exec bit cleared.  (In contrast, the general case dup2()s the descriptor, which does clear close-on-exec.)  So if the fd was close-on-exec, which is the general best practice, it will be closed on exec instead of inherited.

Note that dup2(x, x) is defined as a no-op and POSIX specifically says it won't change the close-on-exec bit, so that will need to be done explicitly.

This *probably* isn't a problem in practice, because the low-valued fds we map things to will *probably* wind up occupied by long-lived fds in the parent process… but maybe not.  In any case I'd like to rule this out as a possible cause of bug 1455828.

Interestingly, Chromium's current version of this code doesn't seem to handle that either, so maybe I'm missing something... or maybe they have a 10-year-old bug.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Another possibility is to just rip out most of this code, and instead dup() as needed to keep the source and destination fd ranges disjoint, at which point everything gets dup2()ed and this isn't a special case.

That could also help for the posix_spawn implementation on Mac, which isn't using this and should be (that's another bug I'm in the middle of filing), because I've heard that OS X doesn't like identity mappings with posix_spawn_file_actions_adddup2 (which, if true, is an OS bug, and also I have no idea how it interacts with POSIX_SPAWN_CLOEXEC_DEFAULT).

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I've tested this, and an identity-mapped close-on-exec file does in fact fail to be inherited.


As for comment #1:

From looking at the kernel code, OS X's implementation of posix_spawn_file_actions_adddup2 seems to correct for the old==new case, and it ought to exempt the fd from POSIX_SPAWN_CLOEXEC_DEFAULT, but I haven't tested that yet.

But, the current file_descriptor_shuffle assumes it can dup() in the child process, which will be a problem for bug 1456919, so that's another reason to rewrite.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1456911 - Prelude: Fix unified build breakage exposed by the next patch.

This directory has a number of places where files unintentionally depend
on `#include`s and `using` directives and forward declarations in other
files in the same unified build group.  Adding a file shifts the group
boundaries and exposes some of those bugs; this patch fixes them (but
there are others).

Review commit: https://reviewboard.mozilla.org/r/249864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249864/

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1456911 - Rewrite the fd shuffling to be simpler & handle identity mappings correctly.

This replaces some old Chromium code that tries to minimally disentangle
an arbitrary file descriptor mapping with simpler algorithm, for several
reasons:

1. Do something appropriate when a file descriptor is mapped to the same
fd number in the child; currently they're ignored, which means they'll
be closed if they were close-on-exec.  This implementation duplicates
the fd twice in that case, which seems to be uncommon in practice; this
isn't maximally efficient but avoids special-case code.

2. Make this more generally applicable; the previous design is
specialized for arbitrary code running between fork and exec, but we
also want to use this on OS X with posix_spawn, which exposes a very
limited set of operations.

3. Avoid the use of C++ standard library iterators in async signal safe
code; the Chromium developers mention that this is a potential problem in
some debugging implementations that take locks.

4. In general the algorithm is simpler and should be more "obviously
correct"; more concretely, it should get complete coverage just by being
run normally in a debug build.

As a convenient side benefit, CloseSuperfluousFds now takes an arbitrary
predicate for which fds to leave open, which means it can be used in
other code that needs it without creating a fake fd mapping.

Review commit: https://reviewboard.mozilla.org/r/249866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249866/

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8983991 [details]
Bug 1456911 - Prelude: Fix unified build breakage exposed by the next patch.

https://reviewboard.mozilla.org/r/249864/#review256226

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8983992 [details]
Bug 1456911 - Rewrite the fd shuffling to be simpler & handle identity mappings correctly.

https://reviewboard.mozilla.org/r/249866/#review256270

This is nice.

::: ipc/chromium/src/base/process_util.h:92
(Diff revision 1)
> -// Close all file descriptors, expect those which are a destination in the
> -// given multimap. Only call this function in a child process where you know
> +// Close all file descriptors, except those for which the given
> +// function returns true (and std{in,out,err}).  Only call this

Maybe rewrite this as "Close all file descriptors, except for std{in,out,err} and those for which the given function returns true."?  I took the current wording to mean the function could return std{in,out,err}, which is admittedly bizarre, and that might have just been me.

::: ipc/glue/FileDescriptorShuffle.h:47
(Diff revision 1)
> +  // Can fail (return false) on failure to duplicate fds.
> +  bool Init(MappingRef aMapping);
> +
> +  // Accessor for the dup2() sequence.  Do not use the returned value
> +  // or the fds contained in it after this object is destroyed.
> +  MappingRef Dup2Sequence() const { return mMapping; }

Should this return value be `const`?

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Thanks for the review.

(In reply to Nathan Froyd [:froydnj] from comment #6)
> ::: ipc/chromium/src/base/process_util.h:92
> (Diff revision 1)
> > -// Close all file descriptors, expect those which are a destination in the
> > -// given multimap. Only call this function in a child process where you know
> > +// Close all file descriptors, except those for which the given
> > +// function returns true (and std{in,out,err}).  Only call this
> 
> Maybe rewrite this as "Close all file descriptors, except for
> std{in,out,err} and those for which the given function returns true."?  I
> took the current wording to mean the function could return std{in,out,err},
> which is admittedly bizarre, and that might have just been me.

Yeah, that looks a little odd; I'll reword.

> ::: ipc/glue/FileDescriptorShuffle.h:47
> (Diff revision 1)
> > +  // Can fail (return false) on failure to duplicate fds.
> > +  bool Init(MappingRef aMapping);
> > +
> > +  // Accessor for the dup2() sequence.  Do not use the returned value
> > +  // or the fds contained in it after this object is destroyed.
> > +  MappingRef Dup2Sequence() const { return mMapping; }
> 
> Should this return value be `const`?

Making the Span itself const won't change anything, I don't think?  And the pointer target type is already const.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8983991 [details]
Bug 1456911 - Prelude: Fix unified build breakage exposed by the next patch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249864/diff/1-2/

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8983992 [details]
Bug 1456911 - Rewrite the fd shuffling to be simpler & handle identity mappings correctly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249866/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/808a2fe2aaab
Prelude: Fix unified build breakage exposed by the next patch. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/69ffa8eab9d9
Rewrite the fd shuffling to be simpler & handle identity mappings correctly. r=froydnj

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/808a2fe2aaab
https://hg.mozilla.org/mozilla-central/rev/69ffa8eab9d9