1400051 - Remove use of base::SetAllFDsToCloseOnExec from process_util_bsd.cc

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P3)

Description

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

This is one half of bug 1400042: process_util_bsd.cc is using base::SetAllFDsToCloseOnExec and potentially leaking concurrently created fds into child processes because it's using posix_spawn, which can't atomically close unwanted fds.  (Apple has a nonstandard extension, the flag POSIX_SPAWN_CLOEXEC_DEFAULT, which does this, but it doesn't seem to be present on other imeplementations.)

As far as I'm aware, *BSD could just use fork/exec the same way as on Linux.  In particular, once I've removed the leftover B2G ChildPrivileges stuff in bug 1316153, process_util_linux.cc could be copied more or less verbatim.

Comment 1

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

I have a patch for this….

Comment 2

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

I've been delaying this because I want to fix the known bug in the “Linux” implementation before saying that it's a good idea to switch the BSDs to it.

(On the other hand, the current BSD implementation is also incorrect in edge cases — see bug 1456919.)

Comment 3

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

Attachment: Bug 1400051 - IPC: use process_util_linux on BSD and remove process_util_bsd.

This change is mainly to avoid the use of SetAllFDsToCloseOnExec, which
has an unavoidable race condition that can leak file descriptors into
child processes.  The "Linux" implementation of child process creation,
now that B2G support has been removed, is essentially a generic Unix
fork+dup2+execve which works on BSD (and is already used on Solaris).

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

Comment 4

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

Comment on attachment 8992055 [details]
Bug 1400051 - IPC: use process_util_linux on BSD and remove process_util_bsd.

r?glandium because this is almost entirely a build change.

f? what I hope is a reasonable set of BSD representatives because I don't want to just barge in and change their stuff without warning.

I've tested locally on FreeBSD 11, and ktraced to make sure that CloseSuperfluousFds is doing something reasonable.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8992055 [details]
Bug 1400051 - IPC: use process_util_linux on BSD and remove process_util_bsd.

https://reviewboard.mozilla.org/r/256976/#review264298

Comment 6

[Landry Breuil (:gaston)]

Comment on attachment 8992055 [details]
Bug 1400051 - IPC: use process_util_linux on BSD and remove process_util_bsd.

m-c builds fine with this diff, and nightly starts with 3 content processes. I've been able to browse a bit with it so i guess e10s works fine, but i dunno what codepaths are precisely affected by the change.

Comment 7

[Jan Beich]

Comment on attachment 8992055 [details]
Bug 1400051 - IPC: use process_util_linux on BSD and remove process_util_bsd.

Tested (mozilla-central changeset b5152aa87782 snapshot) on FreeBSD 10.4 i386 and 11.1 amd64. Both plugin (Adobe Flash) and content processes appear to work fine.

Not possible to test on DragonFly as mozilla-central requires Rust 1.27.0 but only 1.25.0 is packaged at the moment.
https://github.com/DragonFlyBSD/DeltaPorts/tree/master/ports/lang/rust/
http://muscles.dragonflybsd.org/synth/logs/lang___rust.log

Comment 8

[Jan Beich]

Comment on attachment 8992055 [details]
Bug 1400051 - IPC: use process_util_linux on BSD and remove process_util_bsd.

Also tested on FreeBSD 12.0 amd64 both default and --enable-debug build. Content processes work fine. I didn't try Flash plugin this time.

Comment 9

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

Attachment: Patch: use process_util_linux on BSD and remove process_util_bsd. [v2]

Rebased to deal with added comment in process_util_linux; carrying over r+.

Two BSDs confirming this works is probably enough; I'll land it now.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d337c5ca457
IPC: use process_util_linux on BSD and remove process_util_bsd. r=glandium

Comment 11

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/3d337c5ca457