1858279 - [forkserver] Refactor sandbox process launch and clean up forkserver integration

Status: RESOLVED FIXED

(Core :: IPC, task)

Description

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

Back when I added Linux sandboxing features that needed to integrate with process launch by effectively replacing the fork() call, I thought it made sense to make that fully abstract, via a ForkDelegate object rather than by having IPC make a direct call into sandboxing.

Unfortunately, the fork server basically needs to serialize that abstract object and send it to another process. The current approach is kind of hacky — the information is smuggled across in launch parameters that aren't actually used by the launched process, the lifecycle(s) of the SandboxFork object become(s) confusing, and there are ifdef'ed hooks that aren't really necessary.

Also, this blocks bug 1858272 because the sandbox flags are hidden in one of the environment strings.

I have proof-of-concept patches for a fix:

• Replace LaunchOptions::fork_delegate with plain-old-data fields that have the sandboxing info. (Not the best encapsulation but in my opinion better than the alternative. Contrast Chromium, where fork_flags becomes a first-class feature of LaunchOptions and the other parts are handled by an abstract PreExecDelegate; however, Chromium uses a different interface for zygote commands vs. lauching directly.)
• Separate the current SandboxLaunchPrepare into Configure (deciding what sandboxing features are to be used and setting those LaunchOptions fields) and Prepare (actions to be done in the process that will do the clone, before it bakes the FileDescriptorShuffle).
• Adjust the current users of SandboxLaunch/ForkDelegate accordingly.

Currently this seems to work for both the classic and forkserver cases, but it needs more testing and documentation.

As a convenient side-effect, the failing socketpair calls seen in bug 1685642 can now return an error instead of crashing.

(Also, in the forkserver case, that socketpair call moves into the forkserver process (which shouldn't be anywhere near fd exhaustion) rather than the parent; however, bug 1858272 / bug 1752638 will add another parent-process socketpair.)

Comment 1

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

Attachment: Bug 1858279 - Prelude: change Unix LaunchApp to take LaunchOptions by move.

The next patch will need the Linux version of LaunchApp to side-effect
the LaunchOptions in some cases; given that none of the callers ever
try to reuse a LaunchOptions, we might as well change it from a const
reference to an rvalue reference. The Mac version is also changed,
because it uses the same declaration / function type.

Also, the overload of LaunchApp that takes a CommandLine has been
removed, because it was unused.

However, the Windows version is not changed; there's no need for it,
and it accepts different types of arguments anyway so it's not necessary
to match the Unix one. Also, the CommandLine overload of the Windows
LaunchApp is used and continues to exist.

Comment 2

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

Attachment: Bug 1858279 - Rework how process launch interacts with Linux sandboxing and the fork server.

Currently, process launch interfaces with Linux sandboxing via the
ForkDelegate abstraction, basically replacing fork with an opaque
stateful callback, configured using various info from the parent process
(prefs, gfxInfo, etc.). Unfortunately, the fork server effectively
needs to move that object into another process, and this is accomplished
in a way that's complicated and difficult to deal with and causes some
problems.

Instead, this patch makes the sandboxing state transparent: fields
are added to LaunchOptions which are serialized/deserialized, and the
sandbox launcher object is now exposed in a header and used directly by
LaunchApp (and its fork server equivalent).

There are a few other changes that follow from this. In particular,
the pipe for the chroot server is now created later, during LaunchApp
but before FileDescriptorShuffle::Init, so LaunchApp will side-effect
LaunchOptions::fds_to_remap. (But this also means we're no longer
using a fake mapping of fd 10 which isn't actually used, and we're no
longer creating a socketpair in one process and sending both ends to
another process that could have just created it itself.)

For more details, see the comments in SandboxLaunch.h for the member
functions Configure, Prepare, and Fork.

As a convenient side effect of this change, Prepare is now fallible,
so we can handle certain error cases (like failing to create a socket
pair) more gracefully.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b63072009211
Prelude: change Unix LaunchApp to take LaunchOptions by move. r=nika
https://hg.mozilla.org/integration/autoland/rev/bf4c1083cec1
Rework how process launch interacts with Linux sandboxing and the fork server. r=nika,gcp

Comment 4

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/b63072009211
https://hg.mozilla.org/mozilla-central/rev/bf4c1083cec1