diff --git a/dangerzone/isolation_provider/base.py b/dangerzone/isolation_provider/base.py index dcf0280e5..0a46ce09a 100644 --- a/dangerzone/isolation_provider/base.py +++ b/dangerzone/isolation_provider/base.py @@ -30,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) @@ -212,35 +244,6 @@ def terminate_doc_to_pixels_proc( """Terminate gracefully the process started for the doc-to-pixels phase.""" pass - def _signal_process_group(self, 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(self, p: subprocess.Popen) -> None: - """Terminate a process group.""" - if platform.system() == "Windows": - p.terminate() - else: - self._signal_process_group(p, signal.SIGTERM) - - def kill_process_group(self, p: subprocess.Popen) -> None: - """Forcefully kill a process group.""" - if platform.system() == "Windows": - p.kill() - else: - self._signal_process_group(p, signal.SIGKILL) - def ensure_stop_doc_to_pixels_proc( self, document: Document, @@ -272,7 +275,7 @@ def ensure_stop_doc_to_pixels_proc( ) # Forcefully kill the running process. - self.kill_process_group(p) + kill_process_group(p) try: p.wait(timeout_force) except subprocess.TimeoutExpired: diff --git a/dangerzone/isolation_provider/container.py b/dangerzone/isolation_provider/container.py index 7a48b0e28..fe6626c17 100644 --- a/dangerzone/isolation_provider/container.py +++ b/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)) - self.terminate_process_group(p) + terminate_process_group(p) def ensure_stop_doc_to_pixels_proc( # type: ignore [no-untyped-def] self, document: Document, *args, **kwargs diff --git a/dangerzone/isolation_provider/dummy.py b/dangerzone/isolation_provider/dummy.py index 615b04de1..80dd17fcf 100644 --- a/dangerzone/isolation_provider/dummy.py +++ b/dangerzone/isolation_provider/dummy.py @@ -8,7 +8,7 @@ from ..document import Document from ..util import get_resource_path -from .base import IsolationProvider +from .base import IsolationProvider, terminate_process_group log = logging.getLogger(__name__) @@ -86,7 +86,7 @@ def start_doc_to_pixels_proc(self, document: Document) -> subprocess.Popen: def terminate_doc_to_pixels_proc( self, document: Document, p: subprocess.Popen ) -> None: - self.terminate_process_group(p) + terminate_process_group(p) def get_max_parallel_conversions(self) -> int: return 1 diff --git a/tests/isolation_provider/base.py b/tests/isolation_provider/base.py index 762a87654..abf488961 100644 --- a/tests/isolation_provider/base.py +++ b/tests/isolation_provider/base.py @@ -140,7 +140,7 @@ def test_linger_kill( terminate_proc_mock = mocker.patch.object( provider, "terminate_doc_to_pixels_proc", return_value=None ) - kill_pg_spy = mocker.spy(provider, "kill_process_group") + kill_pg_spy = mocker.spy(base, "kill_process_group") with provider.doc_to_pixels_proc(doc, timeout_grace=0) as proc: pass @@ -165,8 +165,8 @@ def test_linger_unkillable( terminate_proc_mock = mocker.patch.object( provider, "terminate_doc_to_pixels_proc", return_value=None ) - kill_pg_mock = mocker.patch.object( - provider, "kill_process_group", return_value=None + kill_pg_mock = mocker.patch( + "dangerzone.isolation_provider.base.kill_process_group", return_value=None ) with provider.doc_to_pixels_proc(doc, timeout_grace=0, timeout_force=0) as proc: