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

Status: NEW

(Core :: IPC, enhancement, P3)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦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-7⟩ ⟦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-7⟩ ⟦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-7⟩ ⟦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.