1456919 - posix_spawn-based LaunchApp implementations aren't handling dup2 hazards

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

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

The fork/exec LaunchApp uses the code in file_descriptor_shuffle.cc to deal with hazards (in the CPU pipeline sense) between dup2 operations — e.g., one mapping's destination happens to be a later one's source.  The posix_spawn implementations aren't, but posix_spawn_file_actions_adddup2 is specified to behave as if a sequence of dup2 calls was made, so it would have the same problems.

This probably isn't biting us for the same reason bug 1456911 isn't biting us — the low(ish)-numbered fds we use as destinations are probably already taken in the parent process, so new fds probably won't overlap.  But the key word there is *probably*; for all we know it could be happening.  And it's not a good idea to leave this kind of footgun lying around anyway.

(Note: non-Mac BSD is also using posix_spawn currently, but that will change in bug 1400051.)

Comment 1

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

A prerequisite for this is changing the file_descriptor_shuffle implementation to not expect the equivalent of dup() in the child.  But I have an idea for that, and it would also take care of bug 1456911 at the same time.

Comment 2

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

For the record, process_util_mac *did* handle identity mappings (src == dest) by clearing close-on-exec, but then I accidentally broke it in bug 1400061; I should've added a posix_spawn_file_actions_addinherit_np in that case, but I didn't, and nobody caught it.

However, it still won't handle other conflicts (e.g., a->b then b->c) and the patch to use the shuffling from bug 1456911 is simple, so I'll do that.

Comment 3

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

Attachment: Bug 1456919 - Shuffle fds correctly in process_util_mac.

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

Comment 4

[Eric Rahm [:erahm]]

Comment on attachment 8992539 [details]
Bug 1456919 - Shuffle fds correctly in process_util_mac.

https://reviewboard.mozilla.org/r/257398/#review264452

Turned out to be much simpler than I expected with `FileDescriptorShuffle`.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5351f1299717
Shuffle fds correctly in process_util_mac. r=erahm

Comment 6

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/5351f1299717