From 756945931f9095a36c7a65d37a90f2482d2526be Mon Sep 17 00:00:00 2001
From: Alex Pyrgiotis <alex.p@freedom.press>
Date: Mon, 1 Jul 2024 17:05:51 +0300
Subject: [PATCH] container: Handle case where `docker kill` hangs

We have encountered several conversions where the `docker kill` command
hangs.  Handle this case by specifying a timeout to this command. If the
timeout expires, log a warning and proceed with the rest of the
termination logic (i.e., kill the conversion process).

Fixes #854
---
 dangerzone/isolation_provider/container.py | 16 ++++++-
 tests/isolation_provider/test_container.py | 51 +++++++++++++++++++++-
 2 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/dangerzone/isolation_provider/container.py b/dangerzone/isolation_provider/container.py
index 5ae8072..4085039 100644
--- a/dangerzone/isolation_provider/container.py
+++ b/dangerzone/isolation_provider/container.py
@@ -15,6 +15,9 @@ 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
 
+TIMEOUT_KILL = 5  # Timeout in seconds until the kill command returns.
+
+
 # Define startupinfo for subprocesses
 if platform.system() == "Windows":
     startupinfo = subprocess.STARTUPINFO()  # type: ignore [attr-defined]
@@ -308,8 +311,19 @@ class Container(IsolationProvider):
             # have stopped right before invoking this command. In that case, the
             # command's output will contain some error messages, so we capture them in
             # order to silence them.
+            #
+            # NOTE: We specify a timeout for this command, since we've seen it hang
+            # indefinitely for specific files. See:
+            # https://github.com/freedomofpress/dangerzone/issues/854
             subprocess.run(
-                cmd, capture_output=True, startupinfo=get_subprocess_startupinfo()
+                cmd,
+                capture_output=True,
+                startupinfo=get_subprocess_startupinfo(),
+                timeout=TIMEOUT_KILL,
+            )
+        except subprocess.TimeoutExpired:
+            log.warning(
+                f"Could not kill container '{name}' within {TIMEOUT_KILL} seconds"
             )
         except Exception as e:
             log.exception(
diff --git a/tests/isolation_provider/test_container.py b/tests/isolation_provider/test_container.py
index 860cf9f..35e9cce 100644
--- a/tests/isolation_provider/test_container.py
+++ b/tests/isolation_provider/test_container.py
@@ -3,7 +3,10 @@ import subprocess
 import time
 
 import pytest
+from pytest_mock import MockerFixture
 
+from dangerzone.document import Document
+from dangerzone.isolation_provider import base
 from dangerzone.isolation_provider.container import Container
 from dangerzone.isolation_provider.qubes import is_qubes_native_conversion
 
@@ -51,4 +54,50 @@ class TestContainer(IsolationProviderTest):
 
 
 class TestContainerTermination(IsolationProviderTermination):
-    pass
+
+    def test_linger_runtime_kill(
+        self,
+        provider_wait: base.IsolationProvider,
+        mocker: MockerFixture,
+    ) -> None:
+        # Check that conversions that remain stuck on `docker|podman kill` are
+        # terminated forcefully.
+        doc = Document()
+        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")
+
+        # Switch the subprocess.run() function with a patched function that
+        # intercepts the `kill` command and switches it with `wait` instead. This way,
+        # we emulate a `docker|podman kill` command that has hang.
+        orig_subprocess_run = subprocess.run
+
+        def patched_subprocess_run(*args, **kwargs):  # type: ignore [no-untyped-def]
+            assert len(args) == 1
+            cmd = args[0]
+            if cmd[1] == "kill":
+                # Switch the `kill` command with `wait`, thereby triggering a timeout.
+                cmd[1] = "wait"
+
+                # Make sure that a timeout has been specified, and make it 0, so that
+                # the test ends us quickly as possible.
+                assert "timeout" in kwargs
+                kwargs[timeout] = 0
+
+                # Make sure that the modified command times out.
+                with pytest.raises(subprocess.TimeoutExpired):
+                    orig_subprocess_run(cmd, **kwargs)
+            else:
+                return orig_subprocess_run(*args, **kwargs)
+
+        mocker.patch("subprocess.run", patched_subprocess_run)
+
+        with provider_wait.doc_to_pixels_proc(doc, timeout_grace=0) as proc:
+            # We purposefully do nothing here, so that the process remains running.
+            pass
+
+        get_proc_exception_spy.assert_not_called()
+        terminate_proc_spy.assert_called()
+        popen_kill_spy.assert_called()
+        assert proc.poll() is not None