diff --git a/dangerzone/isolation_provider/base.py b/dangerzone/isolation_provider/base.py index b8bc3c0..0f76571 100644 --- a/dangerzone/isolation_provider/base.py +++ b/dangerzone/isolation_provider/base.py @@ -195,6 +195,13 @@ class IsolationProvider(ABC): def start_doc_to_pixels_proc(self, document: Document) -> subprocess.Popen: pass + @abstractmethod + def terminate_doc_to_pixels_proc( + self, document: Document, p: subprocess.Popen + ) -> None: + """Terminate gracefully the process started for the doc-to-pixels phase.""" + pass + # From global_common: diff --git a/dangerzone/isolation_provider/container.py b/dangerzone/isolation_provider/container.py index 70da3ef..240b597 100644 --- a/dangerzone/isolation_provider/container.py +++ b/dangerzone/isolation_provider/container.py @@ -215,6 +215,28 @@ class Container(IsolationProvider): args = [container_runtime] + args return self.exec(args) + def kill_container(self, name: str) -> None: + """Terminate a spawned container. + + We choose to terminate spawned containers using the `kill` action that the + container runtime provides, instead of terminating the process that spawned + them. The reason is that this process is not always tied to the underlying + container. For instance, in Docker containers, this process is actually + connected to the Docker daemon, and killing it will just close the associated + standard streams. + """ + container_runtime = self.get_runtime() + cmd = [container_runtime, "kill", name] + try: + subprocess.run(cmd, capture_output=True, check=True) + except subprocess.CalledProcessError as e: + log.warning(f"Failed to kill container {name}: {str(e)}") + log.warning(f"Output from the kill command: {e.output}") + except Exception as e: + log.exception( + f"Unexpected error occurred while killing container {name}: {str(e)}" + ) + def pixels_to_pdf( self, document: Document, tempdir: str, ocr_lang: Optional[str] ) -> None: @@ -273,6 +295,11 @@ class Container(IsolationProvider): name = self.doc_to_pixels_container_name(document) return self.exec_container(command, name=name) + def terminate_doc_to_pixels_proc( + self, document: Document, p: subprocess.Popen + ) -> None: + self.kill_container(self.doc_to_pixels_container_name(document)) + def get_max_parallel_conversions(self) -> int: # FIXME hardcoded 1 until length conversions are better handled # https://github.com/freedomofpress/dangerzone/issues/257 diff --git a/dangerzone/isolation_provider/dummy.py b/dangerzone/isolation_provider/dummy.py index af5a263..aaf718f 100644 --- a/dangerzone/isolation_provider/dummy.py +++ b/dangerzone/isolation_provider/dummy.py @@ -77,5 +77,10 @@ class Dummy(IsolationProvider): def start_doc_to_pixels_proc(self, document: Document) -> subprocess.Popen: return subprocess.Popen("True") + def terminate_doc_to_pixels_proc( + self, document: Document, p: subprocess.Popen + ) -> None: + pass + def get_max_parallel_conversions(self) -> int: return 1 diff --git a/dangerzone/isolation_provider/qubes.py b/dangerzone/isolation_provider/qubes.py index b26d390..f8f86bd 100644 --- a/dangerzone/isolation_provider/qubes.py +++ b/dangerzone/isolation_provider/qubes.py @@ -77,6 +77,32 @@ class Qubes(IsolationProvider): return p + def terminate_doc_to_pixels_proc( + self, document: Document, p: subprocess.Popen + ) -> None: + """Terminate a spawned disposable qube. + + Qubes does not offer a way out of the box to terminate disposable Qubes from + domU [1]. Our best bet is to close the standard streams of the process, and hope + that the disposable qube will attempt to read/write to them, and thus receive an + EOF. + + There are two ways we can do the above; close the standard streams explicitly, + or terminate the process. The problem with the latter is that terminating + `qrexec-client-vm` happens immediately, and we no longer have a way to learn if + the disposable qube actually terminated. That's why we prefer closing the + standard streams explicitly, so that we can afterwards use `Popen.wait()` to + learn if the qube terminated. + + [1]: https://github.com/freedomofpress/dangerzone/issues/563#issuecomment-2034803232 + """ + if p.stdin: + p.stdin.close() + if p.stdout: + p.stdout.close() + if p.stderr: + p.stderr.close() + def teleport_dz_module(self, wpipe: IO[bytes]) -> None: """Send the dangerzone module to another qube, as a zipfile.""" # Grab the absolute file path of the dangerzone module.