subprocess.Popen.send_signal() should poll the process

[vstinner]

BPO	38630
Nosy	@vstinner, @giampaolo, @njsmith, @SpecLad, @oconnor663
PRs	bpo-38630: subprocess: enhance send_signal() on Unix #16984

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-01-15.16:53:35.210>
created_at = <Date 2019-10-29.11:18:26.776>
labels = ['library', '3.9']
title = 'subprocess.Popen.send_signal() should poll the process'
updated_at = <Date 2020-12-03.17:05:56.445>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2020-12-03.17:05:56.445>
actor = 'oconnor663'
assignee = 'none'
closed = True
closed_date = <Date 2020-01-15.16:53:35.210>
closer = 'vstinner'
components = ['Library (Lib)']
creation = <Date 2019-10-29.11:18:26.776>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 38630
keywords = ['patch']
message_count = 25.0
messages = ['355645', '355647', '355706', '355856', '355917', '355918', '355928', '356073', '356074', '356075', '356076', '356077', '356084', '356085', '356571', '356593', '358137', '358142', '358146', '358147', '360064', '360065', '382382', '382397', '382430']
nosy_count = 5.0
nosy_names = ['vstinner', 'giampaolo.rodola', 'njs', 'SpecLad', 'oconnor663']
pr_nums = ['16984']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue38630'
versions = ['Python 3.9']

[vstinner]

subprocess.Popen.send_signal() doesn't check if the process exited since the poll() method has been called for the last time. If the process exit just before os.kill() is called, the signal can be sent to the wrong process if the process identified is recycled.

Attached PR simply calls poll() once to reduce the time window when this race condition can occur, but it doesn't fully kill the race condition.

--

See also the new "pidfd" API which only landed very recently in the Linux kernel to prevent this issue:

• https://lwn.net/Articles/773459/
  "Toward race-free process signaling" (this articles describes this issue)
• https://lwn.net/Articles/789023/
  "New system calls: pidfd_open() and close_range()"
• https://kernel-recipes.org/en/2019/talks/pidfds-process-file-descriptors-on-linux/
  "pidfds: Process file descriptors on Linux" by Chrisitan Brauner

Illumos, OpenBSD, NetBSD and FreeBSD have similar concepts.

I don't propose to use pidfd here, but it's just to highlight that it's a real issue and that kernels are evolving to provide more reliable solutions against the kill(pid, sig) race condition ;-)

[vstinner]

I noticed this issue by reading subprocess code while working on bpo-38207 ("subprocess: Popen.kill() + Popen.communicate() is blocking until all processes using the pipe close the pipe") and bpo-38502 ("regrtest: use process groups").

[vstinner]

subprocess.Popen.send_signal() doesn't check if the process exited since the poll() method has been called for the last time. If the process exit just before os.kill() is called, the signal can be sent to the wrong process if the process identified is recycled.

I'm not sure under which conditions this case happens.

I understood that the pid is released (and so can be reused) as soon as the child process completes *and* waitpid() has been called.

Two cases:

• os.waitpid(pid, 0) can be called directly ("raw call")
• Popen API used, but called from a different thread: thread A calls send_signal(), thread B calls waitpid()

I'm mostly concerned by the multithreading case. I noticed the race condition while working on regrtest which uses 2 threads. One thread which is responsible for the process (call .communicate(), call .wait(), etc.), and one "main" thread which sends a signal to every child processes to interrupt them.

My first implementation called .wait() after calling .send_signal() in the main thread. But it seems like Popen is not "fully" thread safe: the "waitpid lock" only protects the os.waitpid() call, it doesn't protect the self.returncode attribute.

I modified regrtest to only call .waitpid() in the thread responsible of the process, to avoid such race condition. It seems like the new design is more reliable. The main thread only calls .send_signal(): it doesn't call .wait() (which calls os.waitpid()) anymore.

[giampaolo]

-1 about the PR solution to suppress ProcessLookupError in case the process is gone. In psutil I solved the “pid reused problem” by using process creation time to identify a process uniquely (on start).
A decorator can be used to protect the sensibile methods interacting with the PID/handle (communicate(), send_signal(), terminate(), kill()) and raise an exception (maybe ProcessLooKupError(“PID has been reused”)?).

[vstinner]

-1 about the PR solution to suppress ProcessLookupError in case the process is gone.

Did someone propose a pull request to fix this issue by ignoring ProcessLookupError?

In psutil I solved the “pid reused problem” by using process creation time to identify a process uniquely (on start).

Well, that requires to write platform specific code.

I would prefer to only use platform specific code when the platform provides something like pidfd: I understand that Linux, FreeBSD, OpenBSD, NetBSD, and Illumos are concerned. I mean reuse an exisiting solution, rather than writing our own "workaround" (solution?).

[vstinner]

I would prefer to only use platform specific code when the platform provides something like pidfd: I understand that Linux, FreeBSD, OpenBSD, NetBSD, and Illumos are concerned. I mean reuse an exisiting solution, rather than writing our own "workaround" (solution?).

By the way, I suggest to stick to the initial problem in this issue. If someone is intersted to explore the pidfd idea, I recommend to open a different issue ;-)

[giampaolo]

Did someone propose a pull request to fix this issue by ignoring ProcessLookupError?

I misread your PR, sorry. I thought that was the effect.

[njsmith]

It sounds like there's actually nothing to do here? (Except maybe eventually switch to pidfd or similar, but Victor says he wants to use a different issue for that.) Can this be closed?

[vstinner]

It sounds like there's actually nothing to do here? (Except maybe eventually switch to pidfd or similar, but Victor says he wants to use a different issue for that.) Can this be closed?

I opened this issue to propose PR 16984. Did you notice the PR?

In short, I propose to call poll() before calling os.kill() in send_signal().

[njsmith]

But then deeper in the thread it sounded like you concluded that this didn't actually help anything, which is what I would expect :-).

[vstinner]

But then deeper in the thread it sounded like you concluded that this didn't actually help anything, which is what I would expect :-).

The PR contains a test which demonstrate one race condition. It fails without the fix.

[vstinner]

(Except maybe eventually switch to pidfd or similar, but Victor says he wants to use a different issue for that.)

pidfd is not available on all platforms (ex: Windows).

[njsmith]

You can't solve a time-of-check-to-time-of-use race by adding another check. I guess your patch might narrow the race window slightly, but it was a tiny window before and it's a tiny window after, so I don't really get it.

The test doesn't demonstrate a race condition. What it demonstrates is increased robustness against user code that corrupts Popen's internal state by reaping one of its processes behind its back.

That kind of robustness might be a good motivation on its own! (I actually just did a bunch of work to make trio more robust against user code closing fds behind its back, which is a similar kind of thing.) But it's a very different motivation than what you say in this bug.