Skip to content

Commit

Permalink
python: fix signals on SubprocessTransport
Browse files Browse the repository at this point in the history
The methods .send_signal(), .terminate(), and .kill() on
SubprocessTransport are all potentially racy because they enter the
signal handling code on the subprocess object, which contains a
waitpid() call which conflicts with our pidfd in the event that we're
signalling a process that has very recently exited (ie: before we could
notice it).

Fix that by sending the signal ourselves in .send_signal(), and
modifying the other two methods to use .send_signal().
  • Loading branch information
allisonkarlitskaya committed Feb 16, 2023
1 parent 3a4eece commit 95bffd5
Showing 1 changed file with 24 additions and 4 deletions.
28 changes: 24 additions & 4 deletions src/cockpit/transports.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import logging
import os
import select
import signal
import socket
import struct
import subprocess
Expand Down Expand Up @@ -381,14 +382,33 @@ def get_returncode(self) -> Optional[int]:
def get_pipe_transport(self, fd: int) -> asyncio.Transport:
raise NotImplementedError

def send_signal(self, signal: int) -> None: # type: ignore # https://github.com/python/mypy/issues/13885
self._process.send_signal(signal)
def send_signal(self, sig: signal.Signals) -> None: # type: ignore # https://github.com/python/mypy/issues/13885
# We try to avoid using subprocess.send_signal(). It contains a call
# to waitpid() internally to avoid signalling the wrong process (if a
# PID gets reused), but:
#
# - we already detect the process exiting via our PidfdChildWatcher
#
# - the check is actually harmful since collecting the process via
# waitpid() prevents the PidfdChildWatcher from doing the same,
# resulting in an error.
#
# It's on us now to check it, but that's easy:
if self._returncode is not None:
logger.debug("won't attempt %s to process %i. It exited already.", sig, self._process.pid)

try:
os.kill(self._process.pid, sig)
logger.debug('sent %s to process %i', sig, self._process.pid)
except ProcessLookupError:
# already gone? fine
logger.debug("can't send %s to process %i. It's exited just now.", sig, self._process.pid)

def terminate(self) -> None:
self._process.terminate()
self.send_signal(signal.SIGTERM)

def kill(self) -> None:
self._process.kill()
self.send_signal(signal.SIGKILL)

def _close(self) -> None:
if self._pty_fd is not None:
Expand Down

0 comments on commit 95bffd5

Please sign in to comment.