227246 - PR_CreateProcess()/PR_WaitProcess() doesn't play nicely with fork() on Unix/Linux

Status: NEW

(NSPR :: NSPR, enhancement)

Description

[Chris Elving]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 (ax)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 (ax)

On Unix/Linux with _PR_NATIVE_THREADS, PR_CreateProcess() increments the
numProcs counter and adds a pr_PidRecord entry to the pidTable. PR_WaitProcess()
inspects pidTable and, if the process has not yet terminated, waits on reapedCV.

The WaitPidDaemonThread() thread monitors numProcs and, when it's non-zero,
calls waitpid(-1, ...). When waitpid(-1, ...) returns, WaitPidDaemonThread()
decrements numProcs regardless of whether the given pid was in pidTable. If the
given pid was in pidTable, WaitPidDaemonThread() notifies reapedCV, potentially
waking a PR_WaitProcess().

As a result of the above logic, if any code in the process fork()s a process
without using PR_CreateProcess() then 1. PR_WaitProcess() can hang forever and
2. the non-NSPR-based code can fail to properly reap its own children. This is
especially problematic in an environment such as a web server that uses
PR_CreateProcess() and also supports 3rd party plugins.

With _PR_NATIVE_THREADS, I think PR_WaitProcess() should call
waidpid(process->md.pid, ...) directly. This would solve both of the above problems.


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Karl Tomlinson (:karlt)]

Issue 2 exists even without PR_NATIVE_THREADS.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/md/unix/uxproces.c&rev=3.24&mark=580,601,670#612

Comment 3

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

Bug 745212 has a more detailed analysis of the bug.

I expect that Linux has supported _PR_NATIVE_THREADS since at least the advent of NPTL, which was something like a decade ago now (so, it wouldn't have helped when this bug was filed, but fortunately now is not then).  Given a waitpid() that actually works, the NSPR "proces" API maps more or less directly onto Unix… except for PR_DetachProcess/PR_CreateProcessDetached.

One alternative would be for those functions to create a detached pthread for each detached child, to wait on it.  I don't know if we'd consider that to be excessive overhead.

Another alernative is for the wait-daemon thread, when SIGCHLD is posted, to loop over all running children doing waitpid with WNOHANG on each one.  (If non-detached children are using waitpid directly, this could be limited to each running detached child.)  I also don't know if we'd consider that to be excessive overhead.

Or someone could go through Gecko switching all the callers of PR_CreateProcess* to something else.  I'm not sure what that would be.

Comment 4

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

I just tripped over another failure mode from this.  Gecko's IPC code uses libevent to temporarily catch SIGCHLD when waiting for a process to exit; this installs a signal handler which apparently doesn't invoke the previous handler, although it does restore it afterwards.  So, if this races with an NSPR child process exiting, NSPR's SIGCHLD handler won't get called and PR_WaitProcess will block forever.  (I can reproduce it fairly reliably on a debug build by opening the browser and immediately navigating to about:preferences, which runs shell commands from the mailcap file while enumerating MIME type handlers.)

Comment 5

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

I think I know how to fix the problem in comment #4, and probably also bug 1299581.

Kai, does NSPR still need to support Linux 2.4?  More specifically: does NSPR still need to support LinuxThreads, or can it assume NPTL?

Comment 6

[Wan-Teh Chang]

Hi Jed: Please wait for Kai's reply. But I guess (or at least hope)
NSPR can drop support for LinuxThreads and assume NPTL.

Comment 7

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

On further inspection, it's not quite that simple.  The _PR_NATIVE_THREADS variant of uxproces.c calls waitpid() if and only if pr_wp.numProcs is nonzero.  But, if waitpid returns a non-NSPR child process, then numProcs can be decremented back to zero when there are still NSPR processes outstanding… if I'm reading this correctly; I haven't observed this bug in practice yet.

I'd been wondering about the possibility of introducing a PR_ImportProcess for Gecko IPC to use in place of waitpid, but there are also system libraries that spawn processes, so that wouldn't eliminate the problem.

For my earlier idea of mapping PR_WaitProcess directly to waitpid (which effectively requires _PR_NATIVE_THREADS) and not starting the daemon thread until/unless detached processes are used, there's some awkwardness around what happens if a thread is blocked in PR_WaitProcess and a child process is detached, but I think it would be enough enough for PR_WaitProcess to check the pid table if the targeted waitpid() fails with ECHILD.


And then there's still the other option: bypassing this bug by changing Gecko to use native APIs on generic Unix, like what it already does in most cases on Windows and OS X.  Of course, that wouldn't help anything else that's using NSPR.

Comment 8

[Wan-Teh Chang]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #7)
>
> And then there's still the other option: bypassing this bug by changing
> Gecko to use native APIs on generic Unix, like what it already does in most
> cases on Windows and OS X.

This option is worth considering.

Comment 9

[Kai Engert (:KaiE:)]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #5)
> Kai, does NSPR still need to support Linux 2.4?  More specifically: does
> NSPR still need to support LinuxThreads, or can it assume NPTL?

It seems acceptable to declare that future NSPR/NSS versions can no longer work on Linux 2.4 and classic LinuxThreads.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

I've been experiencing an intermittent test failure in a very specific scenario in bug 1393800 where PR_WaitProcess() gets stuck waiting for a process that I'm sure has terminated correctly. It gets stuck here to be precise:

https://dxr.mozilla.org/mozilla-central/rev/19b32a138d08f73961df878a29de6f0aad441683/nsprpub/pr/src/md/unix/uxproces.c#824

I believe I'm hitting this bug since the test involves crashing and re-spawning a content process in quick succession, and that uses fork() directly IIRC. At the same time the minidump-analyzer tool is being launched via nsIProcess to analyze the minidump left by the crash. Since the latter uses PR_CreateProcess()/PR_WaitProcess() I have the feeling I'm hitting this. It happens only in Linux 64-bit code-coverage builds and only for this test, roughly 2-3 times out of 10.

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

I've just managed to confirm that this bug is causing the intermittent failures I'm seeing in bug 1393800.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.