Kill the process group when conversion terminates

Instead of killing just the invoked Podman/Docker/qrexec process, kill
the whole process group, to make sure that other components that have
been spawned die as well. In the case of Podman, conmon is one of the
processes that lingers, so that's one way to kill it.

dangerzone/isolation_provider/base.py

@@ -1,5 +1,8 @@
 import contextlib
 import logging
+import os
+import platform
+import signal
 import subprocess
 import sys
 import tempfile
@@ -27,6 +30,38 @@
 TIMEOUT_FORCE = 5
 
 
+def _signal_process_group(p: subprocess.Popen, signo: int) -> None:
+    """Send a signal to a process group."""
+    try:
+        os.killpg(os.getpgid(p.pid), signo)
+    except (ProcessLookupError, PermissionError):
+        # If the process no longer exists, we may encounter the above errors, either
+        # when looking for the process group (ProcessLookupError), or when trying to
+        # kill a process group that no longer exists (PermissionError)
+        return
+    except Exception:
+        log.exception(
+            f"Unexpected error while sending signal {signo} to the"
+            f"document-to-pixels process group (PID: {p.pid})"
+        )
+
+
+def terminate_process_group(p: subprocess.Popen) -> None:
+    """Terminate a process group."""
+    if platform.system() == "Windows":
+        p.terminate()
+    else:
+        _signal_process_group(p, signal.SIGTERM)
+
+
+def kill_process_group(p: subprocess.Popen) -> None:
+    """Forcefully kill a process group."""
+    if platform.system() == "Windows":
+        p.kill()
+    else:
+        _signal_process_group(p, signal.SIGKILL)
+
+
 def read_bytes(f: IO[bytes], size: int, exact: bool = True) -> bytes:
     """Read bytes from a file-like object."""
     buf = f.read(size)
@@ -240,7 +275,7 @@ def ensure_stop_doc_to_pixels_proc(
             )
 
             # Forcefully kill the running process.
-            p.kill()
+            kill_process_group(p)
             try:
                 p.wait(timeout_force)
             except subprocess.TimeoutExpired:

dangerzone/isolation_provider/container.py

@@ -13,7 +13,12 @@
 from ..document import Document
 from ..util import get_tmp_dir  # NOQA : required for mocking in our tests.
 from ..util import get_resource_path, get_subprocess_startupinfo
-from .base import PIXELS_TO_PDF_LOG_END, PIXELS_TO_PDF_LOG_START, IsolationProvider
+from .base import (
+    PIXELS_TO_PDF_LOG_END,
+    PIXELS_TO_PDF_LOG_START,
+    IsolationProvider,
+    terminate_process_group,
+)
 
 TIMEOUT_KILL = 5  # Timeout in seconds until the kill command returns.
 
@@ -436,7 +441,7 @@ def terminate_doc_to_pixels_proc(
         #
         # See also https://github.com/freedomofpress/dangerzone/issues/791
         self.kill_container(self.doc_to_pixels_container_name(document))
-        p.terminate()
+        terminate_process_group(p)
 
     def ensure_stop_doc_to_pixels_proc(  # type: ignore [no-untyped-def]
         self, document: Document, *args, **kwargs

tests/isolation_provider/base.py

@@ -140,14 +140,14 @@ def test_linger_kill(
         terminate_proc_mock = mocker.patch.object(
             provider_wait, "terminate_doc_to_pixels_proc", return_value=None
         )
-        popen_kill_spy = mocker.spy(subprocess.Popen, "kill")
+        kill_pg_spy = mocker.spy(base, "kill_process_group")
 
         with provider_wait.doc_to_pixels_proc(doc, timeout_grace=0) as proc:
             pass
 
         get_proc_exception_spy.assert_not_called()
         terminate_proc_mock.assert_called()
-        popen_kill_spy.assert_called()
+        kill_pg_spy.assert_called()
         assert proc.poll() is not None
 
     def test_linger_unkillable(
@@ -165,8 +165,8 @@ def test_linger_unkillable(
         terminate_proc_mock = mocker.patch.object(
             provider_wait, "terminate_doc_to_pixels_proc", return_value=None
         )
-        popen_kill_mock = mocker.patch.object(
-            subprocess.Popen, "kill", return_value=None
+        kill_pg_mock = mocker.patch(
+            "dangerzone.isolation_provider.base.kill_process_group", return_value=None
         )
 
         with provider_wait.doc_to_pixels_proc(
@@ -176,7 +176,7 @@ def test_linger_unkillable(
 
         get_proc_exception_spy.assert_not_called()
         terminate_proc_mock.assert_called()
-        popen_kill_mock.assert_called()
+        kill_pg_mock.assert_called()
         assert proc.poll() is None
 
         # Reset the function to the original state.

tests/isolation_provider/test_container.py

@@ -65,7 +65,7 @@ def test_linger_runtime_kill(
         provider_wait.progress_callback = mocker.MagicMock()
         get_proc_exception_spy = mocker.spy(provider_wait, "get_proc_exception")
         terminate_proc_spy = mocker.spy(provider_wait, "terminate_doc_to_pixels_proc")
-        popen_kill_spy = mocker.spy(subprocess.Popen, "kill")
+        kill_proc_spy = mocker.spy(base, "kill_process_group")
 
         # Switch the subprocess.run() function with a patched function that
         # intercepts the `kill` command and switches it with `wait` instead. This way,
@@ -98,5 +98,5 @@ def patched_subprocess_run(*args, **kwargs):  # type: ignore [no-untyped-def]
 
         get_proc_exception_spy.assert_not_called()
         terminate_proc_spy.assert_called()
-        popen_kill_spy.assert_called()
+        kill_proc_spy.assert_called()
         assert proc.poll() is not None