Inherit the initial IPC pipe on Windows
Categories
(Core :: IPC, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox116 | --- | fixed |
People
(Reporter: nika, Assigned: nika)
References
Details
Attachments
(4 files)
Currently when a content process wants to connect back to the parent process on Windows, we explicitly pass down a pipe name, and open it from the content process, performing a ConnectNamedPipe. This has the downside of meaning that if a content process crashes before trying to connect the pipe, we need to use an artificial timeout to detect that the channel will never become connected due to the content process crashing (which was implemented in bug 1788173).
On posix platforms, we instead inherit the client file descriptor into the new child process, allowing us to easily detect when the process dies, as it will lead to the closure of the child's file descriptor. Windows also has support for inheriting a specific set of handles into the spawned process which we may want to use instead. This change would also allow some clean-up of windows-specific concepts during IPC::Channel creation such as the ChannelId.
Unfortunately there are some potential downsides to this approach due to how HANDLE inheritance works on Windows. Specifically, in order to inherit a HANDLE it must be marked as HANDLE_FLAG_INHERIT. This is OK for CreateProcess callers which use STARTUPINFOEX to specify the specific list of handles to inherit, but legacy callers which only pass TRUE to bInheritHandles will inherit all handles marked as HANDLE_FLAG_INHERIT into the child process. While we generally avoid doing this in Gecko, this could theoretically lead to the initial IPC channel handle being inherited into a process spawned by a third-party DLL.
If this turns out to be an issue for us, we could look into other possible approaches, including starting the process suspended, DuplicateHandle-ing the handle into it, and then writing the handle's value somewhere into the child process' memory before starting, however in an ideal world we'd be able to avoid that kind of complexity.
|
Assignee | |
Comment 1•1 year ago
|
||
This allows simplifying how IPC::Channel is created and passed between
processes, as all platforms now inherit the initial handle/fd into the content
process in a similar way. To keep things simple for now, I've continued to use
the base::CommandLine class to pass the HANDLE's identity on Windows, however
we may want to change this to make it a bit easier to follow, perhaps treating
it more like how we handle the IPC fd on Android.
|
|
Assignee | |
Comment 2•1 year ago
|
||
Now that we've simplified the startup process somewhat, it is easier to clean
up IPC channel creation for NodeChannel connections. This stops having
GeckoChildProcessHost inherit from IPC::Channel::Listener, as it would never
receive most of the relevant callbacks, and instead implements the one callback
it would receive directly as a method on that type.
Depends on D181281
|
||
Comment 3•1 year ago
|
||
I like the amount of code that can be removed by doing this. But, I'm a little concerned about the possibility of racing with calls to CreateProcess with bInheritHandles == true, because nsIProcess does this via NSPR, and Subprocess.jsm calls it that way directly via js-ctypes, so pretty much all of our non-IPC child process launching is a potential problem. If the impact is just that we'd have a very small probability of not noticing a crashed process, that's… still not great, but if we'd get stuck during normal child process shutdown that's worse.
A few possible approaches:
-
As mentioned: start the process suspended,
DuplicateHandlethe pipe into it, and write theHANDLEinto its memory before starting it. :bobowen mentioned that the sandbox could do that, although it looks likePolicyBase::AddHandleToSharemethod (what we're currently using to pass handles at launch time) does the same thing of marking the handles inheritable and putting them inPROC_THREAD_ATTRIBUTE_HANDLE_LISTthat we do in the non-sandbox launch path. -
Have the process inherit a pipe, but use it only to send the
HANDLEfor the real IPC pipe, which is duplicated into the child process immediately after it's started. This is a little annoying but it should work. -
Attach an
ObjectWatcherto the process so we know if it exits and can raise a channel error or something even if the pipe isn't closed. This may be worse than option #2 in every way that matters, but I thought I'd mention it for completeness.
|
|
Assignee | |
Comment 4•1 year ago
|
||
(In reply to Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ from comment #3)
I like the amount of code that can be removed by doing this. But, I'm a little concerned about the possibility of racing with calls to
CreateProcesswithbInheritHandles == true, becausensIProcessdoes this via NSPR, andSubprocess.jsmcalls it that way directly via js-ctypes, so pretty much all of our non-IPC child process launching is a potential problem.
This isn't entirely true. nsIProcess doesn't use NSPR on windows, it does direct winapi calls (https://searchfox.org/mozilla-central/rev/ddd233d195a5e4b1c75a972a7fd322947fa1d5e3/xpcom/threads/nsProcessCommon.cpp#362-372,401), and from ccov we can actually see that PR_CreateProcess on windows is never called (https://coverage.moz.tools/#revision=latest&path=nsprpub/pr/src/md/windows/ntmisc.c&view=file&line=554).
You are also correct that Subprocess.jsm also calls CreateProcess using js-ctypes, but that caller actually does use STARTUPINFOEXW and specify an explicit list of HANDLEs to inherit (https://searchfox.org/mozilla-central/rev/1d43d9f3d0ffcbdb619cfd1e2911fb22d1456657/toolkit/modules/subprocess/subprocess_worker_win.js#530-531,537,548), so processes launched that way will not racily inherit the handle.
After a quick look, I couldn't actually find any in-use callers in the tree which pass bCreateHandles == true without also passing a STARTUPINFOEXW to specify which handles to inherit, which makes the impact much more limited. I think we'd only end up accidentally inheriting this into the wrong process if a third-party dll or similar injected itself into our process and ran CreateProcess under the hood. This is better than chromium's situation, which as far as I can tell is the same as ours, as they also appear to inherit all handles in a few places from within Chromium code (e.g. https://source.chromium.org/chromium/chromium/src/+/main:base/process/launch_win.cc;l=76-98;drc=bff3a165135ddfb9cc4dad89c0df0d73c25a0952).
In a way, this is actually better than the situation we have with posix file descriptors for initial process pipes, as we use socketpair to create the socket and then set FD_CLOEXEC afterwards (link), meaning that we could inherit the FD across an exec on posix racily as well (I believe there are APIs on more recent kernels which allow us to specify FD_CLOEXEC upon creation, but we don't use them yet). In fact the posix issue is actually potentially more broad, as we could inherit both sides of any pipe, not just the one used to create an initial process connection.
If the impact is just that we'd have a very small probability of not noticing a crashed process, that's… still not great, but if we'd get stuck during normal child process shutdown that's worse.
I don't believe we'd get stuck during normal child process shutdown - it should just be that we wouldn't notice a crashed process. During normal process shutdown, the ports for actors are torn down explicitly.
- As mentioned: start the process suspended,
DuplicateHandlethe pipe into it, and write theHANDLEinto its memory before starting it. :bobowen mentioned that the sandbox could do that, although it looks likePolicyBase::AddHandleToSharemethod (what we're currently using to pass handles at launch time) does the same thing of marking the handles inheritable and putting them inPROC_THREAD_ATTRIBUTE_HANDLE_LISTthat we do in the non-sandbox launch path.
I mostly worry about the extra complexity of doing something like this. It also doesn't extend very well to other types of HANDLEs which may be inherited across process boundaries. Doing something like this is an option though.
- Have the process inherit a pipe, but use it only to send the
HANDLEfor the real IPC pipe, which is duplicated into the child process immediately after it's started. This is a little annoying but it should work.
This would probably work I suppose, though I don't love the extra complexity and need to make the inherited pipe different from other pipes in terms of how it's handled within ipc_channel_win. It is possible though, and should generally avoid this potential issue.
- Attach an
ObjectWatcherto the process so we know if it exits and can raise a channel error or something even if the pipe isn't closed. This may be worse than option #2 in every way that matters, but I thought I'd mention it for completeness.
It might not actually be the worst idea to hook up the NodeChannel for the child process to the ChildReaper on both posix and windows to ensure that once the process exits, we shut down all IPC communication to the process at the same time. IIRC we don't create the reapers until we think the process is already exiting though (due to the pipe having been closed), so this would be a pretty substantial change.
|
|
Assignee | |
Comment 5•1 year ago
|
||
This call is no longer required after the other changes in part 1 and 2,
and adds substantial complexity to the type. This just removes all of
the ConnectNamedPipe handling logic from ipc_channel_win now that it is
unnecessary.
Depends on D181282
|
|
||
Comment 6•1 year ago
|
||
Thanks for the correction about our existing uses of CreateProcess; that's a much better situation than I thought we were in. I think I'm okay with this, given that the scope for leaking the handle is no worse than (and probably better than) where we've been on Unix platforms since the beginning of IPC.
|
|
Assignee | |
Comment 7•1 year ago
|
||
Depends on D181803
|
||
Comment 9•1 year ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/80562fb693e7
https://hg.mozilla.org/mozilla-central/rev/8d1309c411f4
https://hg.mozilla.org/mozilla-central/rev/4f4197a36617
https://hg.mozilla.org/mozilla-central/rev/1e4fae5c50ea
Description
•