1752638 - Fork server: remoting for child process exit handling

Status: NEW

(Core :: IPC, task)

Description

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

Currently, the fork server ignores child processes exiting, and the parent process needs to guess whether the process exited by probing its pid, which can be wrong if the pid is reused (in the worst case, this could mean that we kill -9 an unrelated process). This also doesn't allow seeing the child process's exit status; see also bug 1752625.

Currently, the fork server's MiniTransceiver requires messages to alternate between the parent sending and the parent receiving. I'd suggest removing that, and instead handling synchronous calls by having the parent send the fork server one end of a socketpair or pipe for that message's reply, like the Linux sandbox broker and the crash reporter. The socket should also be changed from SOCK_STREAM to SOCK_SEQPACKET to allow sending atomically from any thread; note that it currently doesn't handle short writes anyway.

The resulting protocol would look like this:

• [parent→forkserver, sync] LaunchApp; takes various parameters, returns pid
• [parent→forkserver, sync] WaitPid; takes pid and whether to block, returns status or error
• [forkserver→parent, async] SigChld; this will need to be connected to ProcessWatcher (which, after bug 1658072, will use the pipe-to-self trick in our code instead of libevent, so writing a byte to that pipe would work).

The remote waitpid being synchronous and done only at the parent's request (not automatically when the SIGCHLD is received) is subtle: before we call it, we know the pid is still in use and can't be reused; afterwards, we know it's not. This lets us (where “us” includes my ProcessWatcher rewrite) safely index child process information by pid and know that there can't be duplicates as long as we remove any information for a process before doing the waitpid.

For the SIGCHLD notifications, the forkserver socket will need to be connected to an event loop; the IPC I/O thread will work. Alternately, the write end of the ProcessWatcher signal pipe could be sent to the fork server process so it can wake up the ProcessWatcher directly; that's kind of hacky but might be simpler. Also, there's going to be some complication as far as when to waitpid a child directly vs. calling the fork server; one possibility is that, for a fork server child, waitpiding it directly will always fail with ECHILD (we know that the pid hasn't been freed for reuse yet), so it could just always try to wait directly and then fall back to messaging the fork server in that case.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

We probably don't want to ship the forkserver in release before we fix this.

Comment 2

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

Attachment: Bug 1752638 - WORK IN PROGRESS - MiniTransceiver/protocol changes

This patch has my half-finished work making some changes to how fork
server communications works. Before it used a stream socket and always
had matched pairs of synchronous request/reply with no interruptions.
This changes the main protocol to use a sequential-packet socket with
asynchronous messages in (eventually) both directions; ones that need
a reply (currently the fork request, later also the wait request) will
attach a socket for that, like what we do in the Linux sandbox and crash
reporter.

That also allows requests to be sent from any thread in the main process
without locking, although that will probably need threadsafe refcouting
for safety. The attached sockets can be stream sockets, e.g. so that
the arg/env strings could exceed the size of the socket buffer if
needed.

What is not done here is to actually implement the new messages.
Remoting waitid should be relatively straightforward, along similar
lines to how the fork request works after this patch. Sending SIGCHLD
back is more interesting: it's possible to sendmsg directly from the
signal handler, but the message would have to be pre-serialized, because
malloc is not safe there; it's also possible to use the pipe-to-self
idiom. In the main process, connecting those messages to the I/O thread
event loop will also need some work.

There is also some subtlety about child processes outliving the fork
server -- currently we intentionally shut it down while child processes
are still running, and it will always be possible for it to crash, so
we'll need some kind of best-effort fallback there.

There are also some FIXME comments in this patch.

And, integration with ProcessWatcher should be aware of D141309.