Prevent Subprocess processes from inheriting extra file descriptors on *nix
Categories
(Toolkit :: Async Tooling, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox105 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
Details
Attachments
(1 file)
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Comment 3•4 years ago
•
|
||
(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #1)
IPC has a not-quite-standard approach to this that works on the OSes we support: https://searchfox.org/mozilla-central/rev/c56f656febb1/ipc/chromium/src/base/process_util_posix.cc#124
That needs to be called between fork and exec, so it's not something that
can be translated directly into js-ctypes, but possibly the JS could call
into base::LaunchApp; see also bug 1406971.
If the only thing it takes is a JS accessible xpcom component that expose these lines, I could try and devote a few spare cycles to write one.
Also, if I read correctly that it's closing everything except stdin, stdout, stderr and the chrootserver file descriptor. Is this still the behavior that needs to be replicated?
Also, if the idea is to call this from JavaScript... aren't we going to have allocations and/or GCs in JavaScript between fork()
and exec()
? That can cause problems, right?
Assignee | ||
Comment 6•4 years ago
|
||
No, it can't be an XPCOM component. The code that needs to call it runs in a worker. And, yes, running JS code in between the fork and exec would be a problem, which is why the API needs to do both in a single operation. That's why we use posix_spawn
currently.
(In reply to Kris Maglione [:kmag] from comment #6)
No, it can't be an XPCOM component. The code that needs to call it runs in a worker. And, yes, running JS code in between the fork and exec would be a problem, which is why the API needs to do both in a single operation. That's why we use
posix_spawn
currently.
Ok, so in this case, if I understand correctly, we cannot use https://searchfox.org/mozilla-central/rev/c56f656febb1/ipc/chromium/src/base/process_util_posix.cc#124, right?
I toyed with the idea of wrapping around fork()
+ exec()
+ closeUnnecessaryFDs()
, and exposing this either through webidl or through js-ctypes, but there's still the risk that concurrent gc could run between fork()
and exec()
. If we could deactivate gc before fork()
and reactivate it only only on the parent, though, I guess we could make it work.
Comment 8•4 years ago
|
||
My idea was to wrap base::LaunchApp
(and the necessary subset of LaunchOptions
) for use by JS, and have it call that the same way it currently calls posix_spawn
. Currently Subprocess.jsm
supports some features that LaunchApp
doesn't (environmentAppend: false
, workdir
, disclaim
; the first of these doesn't seem to be used), but they could be added.
I strongly advise against trying to run JS in the forked child of a multithreaded process. POSIX allows only async-signal-safe operations in that context (in particular: no locking or anything that could indirectly use locking), and we have some hacks so that malloc
more or less works, but in general it's best to do as little as possible, and a JS interpreter is likely to cause problems.
(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #8)
My idea was to wrap
base::LaunchApp
(and the necessary subset ofLaunchOptions
) for use by JS, and have it call that the same way it currently callsposix_spawn
. CurrentlySubprocess.jsm
supports some features thatLaunchApp
doesn't (environmentAppend: false
,workdir
,disclaim
; the first of these doesn't seem to be used), but they could be added.
Does Subprocess.jsm have access to webidl-defined methods? If so, we could create a method ChromeUtils.launchApp
.
Comment 10•3 years ago
|
||
Kris, are you still actively working on this / do we still need to fix this / how important is this?
Comment 11•3 years ago
|
||
Kicking this out of our triage for now, but leaving needinfo in case this is more important for some reason.
Assignee | ||
Comment 12•3 years ago
|
||
I'm not working on it. We do still need to do it. It's hard to say how important it is. We mark a lot of file descriptors as non-inheritable now, so it's not necessarily a huge issue. But there are still some we don't, and there's still a race between creating them and adding the CLOEXEC flag in a fair number of cases. So it's not exactly great as it is either.
Comment 13•3 years ago
|
||
Unexpected file descriptors that I can see with a quick look at lsof
:
- Necko sockets
- Anything received over IPC, notably a lot of shared memory
- Various
.xpi
files and also the omnijars startupCache
files from the profile
So there are still a number of problems here. (IPC should use MSG_CMSG_CLOEXEC
, or set O_CLOEXEC
nonatomically if that's not available; that one, at least, I know how to fix.)
I look at look at what Subprocess.jsm
provides that base::LaunchApp
currently doesn't:
- Replacing the environment instead of modifying the existing one
- The macOS-specific
responsibility_spawnattrs_setdisclaim
feature - Setting the working directory
These are all easy enough to add. The other problem is how to expose them to JS; nsIProcess
would need to add all of those, as well as the ability to assign fds to stdin/out/err (at least for the cases of pipes or /dev/null). Or maybe XPCOM isn't the right tool for that.
Assignee | ||
Comment 14•2 years ago
|
||
(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #13)
These are all easy enough to add. The other problem is how to expose them to JS;
nsIProcess
would need to add all of those, as well as the ability to assign fds to stdin/out/err (at least for the cases of pipes or /dev/null). Or maybe XPCOM isn't the right tool for that.
nsIProcess
has enough other problems that I'd rather start from scratch with something new rather than use it. We also can't use it for Subprocess.jsm
since Subprocess
needs to run in a worker thread for the sake of the IO work it does, so it can't use XPCOM. We could get rid of the JS part entirely, but I think that would be even more reason not to try to build on nsIProcess
Comment 15•2 years ago
|
||
Currently, Subprocess.jsm on Unix uses js-ctypes to call posix_spawn
.
This has some issues, primarily that file descriptors are inherited by
the child process unless explicitly opted-out, which unfortunately a lot
of code doesn't do. This patch changes it to use IPC process launching,
where fd inheritance is opt-in, by:
-
Extending
base::LaunchApp
to handle a few features that Subprocess
needs (setting the process's working directory, specifying the full
environment, and the macOSdisclaim
flag) -
Adding a WebIDL method to
IOUtils
to expose that function to JS
(currently Unix-only; Windows could also be supported but it would
probably want to use a different IDL type for strings, given that the
OS APIs use 16-bit code units). -
Replacing the part of Subprocess that invokes
posix_spawn
(and
related functions) by calling that method; the rest of Subprocess's
machinery to manage pipes and I/O is unchanged. (The Windows backend
is also unchanged; I'm not aware of any functional issues there.)
This results in some dead code, which is removed.
Comment 16•2 years ago
|
||
Comment 17•2 years ago
|
||
bugherder |
Description
•