Rethink how we give file descriptors / handles to child processes

NEW
Unassigned

Status

()

enhancement
P3
normal
a year ago
19 days ago

People

(Reporter: jld, Unassigned)

Tracking

(Blocks 1 bug)

60 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 affected)

Details

(Whiteboard: [overhead:noted])

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.
> 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.)
Priority: -- → P3
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.
Blocks: 1470591
(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.
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.
Whiteboard: [overhead:noted]
See Also: → 1481139
You need to log in before you can comment on or make changes to this bug.