1440207 - Rethink how we give file descriptors / handles to child processes

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P3)

Description

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

Between bug 1407693 and bug 1438678 I'm noticing that we don't have a good story for giving child processes file descriptors/handles for things that need to be outside of IPDL IPC.  Possibilities include:

* On Windows, set the handles to be inherited and communicate the handle value to the process somehow (as an argument or env var, maybe), because they're chosen by the OS.

** …except that I think IPC does something else for the initial IPDL pipe?

* On regular Unix, hard-code a destination fd somewhere and try not to collide with an existing use (there's an assertion, but bug 1436156 broke it), and add it to fds_to_remap.

** On Mac, things are probably a little broken, because we're not using the injective multimap stuff to handle the case of a mapping destination that collides with a later element's source.  Also it's reported that mapping an fd to itself (leaving it open at its current number) doesn't work.  These may become separate bugs.

* On Android, we have GeckoChildProcessHost::LaunchAndroidService, which makes some "interesting" assumptions about the order of elements in fds_to_remap, and a bunch of Java code that passes each of the fds around as separate arguments.  They eventually end up as ParcelFileDescriptor objects, which is part of an interface for passing fds over Binder RPC; they aren't inherited over execve and the fd numbers they eventually wind up with in the child process shouldn't be assumed (so, closer to Windows in this respect, except that the parent doesn't know the numbers either).

This is kind of an error-prone mess and it would be nice to simplify it.  fds_to_remap can also be used to set the child process's stdin/out/err, so we need something for that use case at least, but in general we don't *really* need magic fds ­— in principle they could be passed in env vars, for example.  Or we could still use fd (3+n) for the nth fd on non-Android Unix, but hide that behind an abstraction that does something else on Android (and Windows).

A sketch of one way to abstract things: Have an enum in a header somewhere, maybe in ipc/glue, giving an index to each of these special fds/handles (i.e., one central place to make sure the identifiers don't collide); an API associated with GeckoChildProcessHost (or maybe LaunchOptions?) to supply them, and then something to call in the child process to retrieve the fds/handles.  The platform-specific concerns (e.g., all those Java methods) could just deal with an array of (optional/nullable) descriptors/handles.

Comment 1

[Nicholas Nethercote [inactive]]

> in general we don't *really* need magic fds ­— in principle they could
> be passed in env vars, for example.

In bug 1438678 I'm passing the handles/fds for Windows, Mac, and Linux via -prefsHandle. (Android doesn't use -prefsHandle because it uses the Parcel stuff instead.)

Comment 2

[Kris Maglione [:kmag]]

Let's cleanup this mess before we start working on a fork server, so we don't have to work around it in yet another place.

Comment 3

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

(In reply to Kris Maglione [:kmag] from comment #2)
> Let's cleanup this mess before we start working on a fork server, so we
> don't have to work around it in yet another place.

I wasn't seeing this as being a problem for that — the fork server could get some fds via SCM_RIGHTS and the destination half of the mapping as data, and then apply the mapping like normal on the child side of the fork.  And the fork server is only for desktop Linux and Mac, not Windows or Android[*], so most of comment #0 doesn't intersect it.  Were you thinking of approaching this bug by throwing out the support for arbitrary fd mappings instead of building a layer over it?

[*] If in the future we switch to fork/exec on Android then it's basically the same as desktop Linux and the Android part still doesn't apply.

Comment 4

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

Also, the Mac thing I mentioned in comment #0 is bug 1456919 and the fix should be relatively simple after the fd mapping rewrite in bug 1456911.

Comment 5

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

Here's an idea: assuming a centralized list of all possible handles needed at startup, then on both Windows and Unix we could just pass one command-line flag, -fdMap or something, with a comma-separated list of the fd/handle values in decimal, in the order that they're listed in the big enum.

Both platforms have in-band nulls (-1 on Unix, 0 a.k.a. NULL on Windows) usable to represent handles that aren't passed; any that aren't meaningful on the platform / build type in question (e.g., IPC on Windows) could even be handled this way instead of trying to ifdef them. UniqueFileHandle should be useful here; both GeckoChildProcessHost and whatever global singleton is created in the child process can have an array of them, behind a suitable abstraction.

For Unix, the fds could be 3+n as currently, or use the same values as the parent (just clearing close-on-exec instead of using dup2, similar to Windows handle inheritance), or even be randomized; it won't really matter, although a nondeterministic option would detect accidental hard-coding.

Some of these are for shared memory, so we'll also want to allow adding a length (maybe with syntax like <fd-or-handle>:<length>) and converting to/from base::SharedMemory; this is already being done with the existing collection of ad-hoc solutions.

Android is its own thing, but fundamentally we just need to get an array of optional fds (and optional lengths) to/from Java, message it to the child as we're currently doing with the existing collection of fds, and then it can proceed like normal Unix.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

I have some WIP patches which rework how we do child process handle passing, so claiming the bug.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 1: Stop setting `gArgc` and `gArgv` in content, r=#ipc-reviewers!

It is never used outside of the parent process.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 2: Add a cross-platform DuplicateFileHandle method to MFBT, r=glandium!

This is convenient in cross-platform code which needs to work with
UniqueFileHandle objects. dup is not supported by wasi, so the method isn't
available there.

Depends on D221369

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 3a: Migrate GeckoChildProcessHost callers to use ChildProcessArgs, r=#ipc-reviewers!

This patch implements the majority of the public interface for the new IPC
handle passing design.

The rough design is an expansion of geckoargs to allow passing
UniqueFileHandle arguments to child processes. This replaces the existing
extra options array to make the list of files explicit.

This currently just replaces things which were already passed this way on the
command line from outside of GeckoChildProcessHost. Note that this does not
migrate callers which were not already passing file handles using geckoargs,
and does not implement the actual OS-level support for passing arguments this
way.

Depends on D221370

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 3b: Use GeckoArgs for parentPID and initialChannelID, r=#ipc-reviewers!

This helps unify some logic between platforms, and removes some positional
arguments from the command line, so it ended up in this patch stack, despite
not technically being required to pass FDs directly.

Depends on D221371

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 3c: Pass the crashreporter pipe using geckoargs, r=gsvelto!

The crash reporter pipe was previously passed as a positional argument with
custom code for each platform. On some platforms it is implemented as a file
being passed, which is now directly supported by geckoargs.

Depends on D221372

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 3d: Pass the initial IPC pipe using geckoargs, r=#ipc-reviewers!

The initial IPC pipe was previously the only argument passed as a chromium
command line flag. This changes it to instead be passed using geckoargs,
unifying the platforms, and removing the need for specific handling on every
platform.

Depends on D221373

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 3e: Pass linux sandbox fds using geckoargs, r=#ipc-reviewers!

The other file descriptors which used to use FDs 3 and 4 are no longer going to
be passed with fixed FDs, and will instead be passed using the new geckoargs
system. Due to the low-level and linux-specificity of the linux-sandbox,
migrating it to use geckoargs is a bit more difficult, so it is being left
using manual FD inheritance/remapping.

The environment variable for chroot has also been changed in this patch to
contain the FD to use to communicate with the chroot server. In a future patch,
the chroot server will be unable to use a fixed FD when the process has been
forked from the fork server, and this provides a mechanism for changing this
behaviour.

Depends on D221374

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 3f: Windows support for file geckoargs, r=#ipc-reviewers!

Windows is the simplest platform to support, as HANDLEs are inherited by
identity into the child process, and there's less variance between different
OSes.

This effectively re-implements the existing behaviour used for HANDLE
passing on Windows by passing the handle directly on the command line.

Depends on D221375

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 3g: Basic posix support for file geckoargs, r=#ipc-reviewers!

This effectively mimics the old system using fixed FDs for passing the
arguments down.

An in-memory array of FDs is used to track which FD corresponds with
with file argument, which is initialized to the fixed offsets, in order
to allow platforms which cannot use fixed FDs in their child processes
to specify explicit files being passed down. This will be used in the
fork server, for Android, and for iOS.

Depends on D221376

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 3h: Android support for file geckoargs, r=#geckoview-reviewers!,#ipc-reviewers!

This changes how file descriptors are passed to Android child processes to
support the new ChildProcessArgs model. Instead of explicit lists of FDs to pass
around, a single array of FDs is passed from the parent process into the child
service and then passed into GeckoArgs.

Depends on D221377

Comment 17

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 3i: Fix iOS builds, r=#ipc-reviewers!

The code which decodes the XPC bootstrap message hasn't landed into
mozilla-central yet, so only the serializing side in GeckoChildProcessHost is
updated here.

Depends on D221378

Comment 18

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 3j: ForkServer support for file geckoargs, r=#ipc-reviewers!

After this change, the fork server no longer uses a Dup2Sequence to set
fixed FD offsets when starting child processes, instead using the new
geckoargs system.

Because this was pretty disruptive to thow the fork server works and was
previously integrated into LaunchApp, the forkserver code has generally been
moved out of the chromium codebase, and inlined into the ForkServer files.

As noted in a previous part, the linux sandbox code does not use
geckoargs, as it is low-level. For the kSandboxReporterFileDesc we can
cheat, because every process wants the same FD created once in the
parent process at the same FD offset. As this will also have been passed
to the fork server process when it was created, we can just allow new
child processes forked from the fork server to inherit this FD from the
fork server.

In contrast, the chroot sandbox pipe is actually created within the
forkserver while forking. To handle this, the environment variable used
to signal that the chroot server should be used is used to specify the
FD where the pipe can be found, and is not set to a fixed value in the
fork server.

Finally, the primary IPC pipe used by the fork server is also changed to
be passed using geckoargs into the fork server, and the fork server
being used is made more explicit in GeckoChildProcessHost, as launching
using the fork server now requires different arguments than launching
normally (due to needing to pass down the ChildProcessArgs).

Depends on D221379

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c2ac609c466
Part 1: Stop setting `gArgc` and `gArgv` in content, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/07fbed499fe4
Part 2: Add a cross-platform DuplicateFileHandle method to MFBT, r=glandium
https://hg.mozilla.org/integration/autoland/rev/f3445c3fdd5e
Part 3a: Migrate GeckoChildProcessHost callers to use ChildProcessArgs, r=ipc-reviewers,necko-reviewers,media-playback-reviewers,aosmond,mccr8
https://hg.mozilla.org/integration/autoland/rev/2da0bbfdfee5
Part 3b: Use GeckoArgs for parentPID and initialChannelID, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/30ff6a4db1d0
Part 3c: Pass the crashreporter pipe using geckoargs, r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/6d715586c911
Part 3d: Pass the initial IPC pipe using geckoargs, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/d13799a1dc26
Part 3e: Pass linux sandbox fds using geckoargs, r=ipc-reviewers,jld
https://hg.mozilla.org/integration/autoland/rev/c581a4e8cd15
Part 3f: Windows support for file geckoargs, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/ea81a08ce181
Part 3g: Basic posix support for file geckoargs, r=ipc-reviewers,jld
https://hg.mozilla.org/integration/autoland/rev/b4d283d3e8da
Part 3h: Android support for file geckoargs, r=geckoview-reviewers,ipc-reviewers,owlish,jld
https://hg.mozilla.org/integration/autoland/rev/75a99142ee1c
Part 3i: Fix iOS builds, r=ipc-reviewers,jld
https://hg.mozilla.org/integration/autoland/rev/76d823a34925
Part 3j: ForkServer support for file geckoargs, r=ipc-reviewers,jld
https://hg.mozilla.org/integration/autoland/rev/c9f33dfc1cc5
apply code formatting via Lando

Comment 20

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 4: Fix lint failure in GeckoServiceChildProcess.java

Comment 21

[Cristian Tuns]

Backed out for causing multiple failures

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoServiceChildProcess.java:110:13 | Variable 'fds' should be declared final. (com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck)
• Failure Log2
• Failure line2: error: test failed, to rerun pass -p style --lib
  error: 1 target failed:
  gmake[4]: *** [/builds/worker/checkouts/gecko/config/makefiles/rust.mk:548: force-cargo-test-run] Error 101

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Something like this would fix the rusttest issue. The mach port stuff does the same:

diff --git a/mfbt/UniquePtrExtensions.h b/mfbt/UniquePtrExtensions.h
index c93f76a008711..c5e45f92f7561 100644
--- a/mfbt/UniquePtrExtensions.h
+++ b/mfbt/UniquePtrExtensions.h
@@ -103,6 +103,7 @@ typedef int FileHandleType;
 #  error "Unsupported OS?"
 #endif

+#if !defined(RUST_BINDGEN)
 struct FileHandleHelper {
   MOZ_IMPLICIT FileHandleHelper(FileHandleType aHandle) : mHandle(aHandle) {
 #if defined(XP_UNIX) && (defined(DEBUG) || defined(FUZZING))
@@ -162,6 +163,7 @@ struct FileHandleDeleter {
   using receiver = FileHandleType;
   MFBT_API void operator()(FileHandleHelper aHelper);
 };
+#endif

 #if defined(XP_DARWIN) && !defined(RUST_BINDGEN)
 struct MachPortHelper {
@@ -215,6 +217,7 @@ struct MachPortSetDeleter {
 template <typename T>
 using UniqueFreePtr = UniquePtr<T, detail::FreePolicy<T>>;

+#if !defined(RUST_BINDGEN)
 // A RAII class for the OS construct used for open files and similar
 // objects: a file descriptor on Unix or a handle on Windows.
 using UniqueFileHandle =
@@ -227,6 +230,7 @@ inline UniqueFileHandle DuplicateFileHandle(const UniqueFileHandle& aFile) {
   return DuplicateFileHandle(aFile.get());
 }
 #endif
+#endif

 #if defined(XP_DARWIN) && !defined(RUST_BINDGEN)
 // A RAII class for a Mach port that names a send right.

It's a bit unfortunate but needed due to how we override UniquePtr to be otherwise usable from rust...

Comment 23

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1440207 - Part 5: Disable UniqueFileHandle for RUST_BINDGEN, r=emilio!,glandium!

Apparently rust bindgen uses a hack which assumes all specializations of
UniquePtr have the basic layout and use a pointer member with an empty
deleter.

This incorrect assumption unfortunately comes up for UniqueFileHandle,
which replaces the pointer type using the deleter with a file handle
helper type.

As this fails to build with RUST_BINDGEN, this patch takes the same
approach as bug 1802320, and just disables building these types when
building headers for bindgen.

This does not fix the general issue of bindgen making incorrect
assumptions about the layout of UniquePtr with non-default deleters.
Ideally, all non-default deleters should be made opaque.

Comment 24

[Nika Layzell [:nika] (ni? for response)]

I believe the 2 follow-up parts I have added will fix these failures.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/409db639b50d
Part 1: Stop setting `gArgc` and `gArgv` in content, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/7aa8871e9e3e
Part 2: Add a cross-platform DuplicateFileHandle method to MFBT, r=glandium
https://hg.mozilla.org/integration/autoland/rev/25a973b1e0f5
Part 3a: Migrate GeckoChildProcessHost callers to use ChildProcessArgs, r=ipc-reviewers,necko-reviewers,media-playback-reviewers,aosmond,mccr8
https://hg.mozilla.org/integration/autoland/rev/aa5a1a50b422
Part 3b: Use GeckoArgs for parentPID and initialChannelID, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/bdc0a11f0ca9
Part 3c: Pass the crashreporter pipe using geckoargs, r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/7664f0f62585
Part 3d: Pass the initial IPC pipe using geckoargs, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/7a0410a922e3
Part 3e: Pass linux sandbox fds using geckoargs, r=ipc-reviewers,jld
https://hg.mozilla.org/integration/autoland/rev/33fb31912242
Part 3f: Windows support for file geckoargs, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/e1bf17be60c4
Part 3g: Basic posix support for file geckoargs, r=ipc-reviewers,jld
https://hg.mozilla.org/integration/autoland/rev/74c3288e212b
Part 3h: Android support for file geckoargs, r=geckoview-reviewers,ipc-reviewers,owlish,jld
https://hg.mozilla.org/integration/autoland/rev/a14291928dd2
Part 3i: Fix iOS builds, r=ipc-reviewers,jld
https://hg.mozilla.org/integration/autoland/rev/2049ff50c110
Part 3j: ForkServer support for file geckoargs, r=ipc-reviewers,jld
https://hg.mozilla.org/integration/autoland/rev/fea31ce1c38c
Part 4: Fix lint failure in GeckoServiceChildProcess.java r=geckoview-reviewers,calu
https://hg.mozilla.org/integration/autoland/rev/79d3abe7bd3c
Part 5: Disable UniqueFileHandle for RUST_BINDGEN, r=emilio,glandium
https://hg.mozilla.org/integration/autoland/rev/a2e2c7a7fc7c
apply code formatting via Lando

Comment 26

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/409db639b50d
https://hg.mozilla.org/mozilla-central/rev/7aa8871e9e3e
https://hg.mozilla.org/mozilla-central/rev/25a973b1e0f5
https://hg.mozilla.org/mozilla-central/rev/aa5a1a50b422
https://hg.mozilla.org/mozilla-central/rev/bdc0a11f0ca9
https://hg.mozilla.org/mozilla-central/rev/7664f0f62585
https://hg.mozilla.org/mozilla-central/rev/7a0410a922e3
https://hg.mozilla.org/mozilla-central/rev/33fb31912242
https://hg.mozilla.org/mozilla-central/rev/e1bf17be60c4
https://hg.mozilla.org/mozilla-central/rev/74c3288e212b
https://hg.mozilla.org/mozilla-central/rev/a14291928dd2
https://hg.mozilla.org/mozilla-central/rev/2049ff50c110
https://hg.mozilla.org/mozilla-central/rev/fea31ce1c38c
https://hg.mozilla.org/mozilla-central/rev/79d3abe7bd3c
https://hg.mozilla.org/mozilla-central/rev/a2e2c7a7fc7c