From 472ebe05efb2fdd6df27b1081d5069ec84d3d3de Mon Sep 17 00:00:00 2001 From: deeplow Date: Thu, 25 Jan 2024 14:00:34 +0000 Subject: [PATCH] FIXUP: allow each conversion to have its own proc If we increased the number of parallel conversions, we'd run into an issue where the streams were getting mixed together. This was because the Converter.proc was a single attribute. This breaks it down into a local variable such that this mixup doesn't happen. --- dangerzone/conversion/errors.py | 27 +++++++++++------ dangerzone/isolation_provider/base.py | 42 ++++++++++++-------------- dangerzone/isolation_provider/qubes.py | 4 --- tests/isolation_provider/base.py | 18 +++++++++-- tests/isolation_provider/test_qubes.py | 10 +++--- 5 files changed, 60 insertions(+), 41 deletions(-) diff --git a/dangerzone/conversion/errors.py b/dangerzone/conversion/errors.py index 61e48c253..235ffc9b2 100644 --- a/dangerzone/conversion/errors.py +++ b/dangerzone/conversion/errors.py @@ -1,3 +1,4 @@ +import subprocess from typing import List, Optional, Type, Union # XXX: errors start at 128 for conversion-related issues @@ -7,6 +8,23 @@ MAX_PAGE_HEIGHT = 10000 +class InterruptedConversionException(Exception): + """Data received was less than expected""" + + def __init__(self) -> None: + super().__init__( + "Something interrupted the conversion and it could not be completed." + ) + + +class ConverterProcException(Exception): + """Some exception occurred in the converter""" + + def __init__(self, proc: subprocess.Popen) -> None: + self.proc = proc + super().__init__() + + class ConversionException(Exception): error_message = "Unspecified error" error_code = ERROR_SHIFT @@ -86,15 +104,6 @@ class PageCountMismatch(PagesException): ) -class ConverterProcException(ConversionException): - """Some exception occurred in the converter""" - - error_code = ERROR_SHIFT + 60 - error_message = ( - "Something interrupted the conversion and it could not be completed." - ) - - class UnexpectedConversionError(ConversionException): error_code = ERROR_SHIFT + 100 error_message = "Some unexpected error occurred while converting the document" diff --git a/dangerzone/isolation_provider/base.py b/dangerzone/isolation_provider/base.py index f637ff382..dfdcd8463 100644 --- a/dangerzone/isolation_provider/base.py +++ b/dangerzone/isolation_provider/base.py @@ -27,7 +27,7 @@ def read_bytes(f: IO[bytes], size: int, exact: bool = True) -> bytes: """Read bytes from a file-like object.""" buf = f.read(size) if exact and len(buf) != size: - raise errors.ConverterProcException() + raise errors.InterruptedConversionException() return buf @@ -35,7 +35,7 @@ def read_int(f: IO[bytes]) -> int: """Read 2 bytes from a file-like object, and decode them as int.""" untrusted_int = f.read(INT_BYTES) if len(untrusted_int) != INT_BYTES: - raise errors.ConverterProcException() + raise errors.InterruptedConversionException() return int.from_bytes(untrusted_int, "big", signed=False) @@ -52,7 +52,6 @@ class IsolationProvider(ABC): def __init__(self) -> None: self.percentage = 0.0 - self.proc: Optional[subprocess.Popen] = None if getattr(sys, "dangerzone_dev", False) == True: self.proc_stderr = subprocess.PIPE @@ -80,8 +79,8 @@ def convert( document.mark_as_safe() if document.archive_after_conversion: document.archive() - except errors.ConverterProcException: - exception = self.get_proc_exception() + except errors.ConverterProcException as e: + exception = self.get_proc_exception(e.proc) self.print_progress(document, True, str(exception), 0) document.mark_as_failed() except errors.ConversionException as e: @@ -96,16 +95,16 @@ def convert( def doc_to_pixels(self, document: Document, tempdir: str) -> None: with open(document.input_filename, "rb") as f: - self.proc = self.start_doc_to_pixels_proc() + p = self.start_doc_to_pixels_proc() try: - assert self.proc.stdin is not None - self.proc.stdin.write(f.read()) - self.proc.stdin.close() + assert p.stdin is not None + p.stdin.write(f.read()) + p.stdin.close() except BrokenPipeError as e: - raise errors.ConverterProcException() + raise errors.ConverterProcException(p) - assert self.proc.stdout - n_pages = read_int(self.proc.stdout) + assert p.stdout + n_pages = read_int(p.stdout) if n_pages == 0 or n_pages > errors.MAX_PAGES: raise errors.MaxPagesException() percentage_per_page = 50.0 / n_pages @@ -114,8 +113,8 @@ def doc_to_pixels(self, document: Document, tempdir: str) -> None: text = f"Converting page {page}/{n_pages} to pixels" self.print_progress(document, False, text, self.percentage) - width = read_int(self.proc.stdout) - height = read_int(self.proc.stdout) + width = read_int(p.stdout) + height = read_int(p.stdout) if not (1 <= width <= errors.MAX_PAGE_WIDTH): raise errors.MaxPageWidthException() if not (1 <= height <= errors.MAX_PAGE_HEIGHT): @@ -123,7 +122,7 @@ def doc_to_pixels(self, document: Document, tempdir: str) -> None: num_pixels = width * height * 3 # three color channels untrusted_pixels = read_bytes( - self.proc.stdout, + p.stdout, num_pixels, ) @@ -138,16 +137,16 @@ def doc_to_pixels(self, document: Document, tempdir: str) -> None: self.percentage += percentage_per_page # Ensure nothing else is read after all bitmaps are obtained - self.proc.stdout.close() + p.stdout.close() # TODO handle leftover code input text = "Converted document to pixels" self.print_progress(document, False, text, self.percentage) if getattr(sys, "dangerzone_dev", False): - assert self.proc.stderr - untrusted_log = read_debug_text(self.proc.stderr, MAX_CONVERSION_LOG_CHARS) - self.proc.stderr.close() + assert p.stderr + untrusted_log = read_debug_text(p.stderr, MAX_CONVERSION_LOG_CHARS) + p.stderr.close() log.info( f"Conversion output (doc to pixels)\n{self.sanitize_conversion_str(untrusted_log)}" ) @@ -173,10 +172,9 @@ def print_progress( if self.progress_callback: self.progress_callback(error, text, percentage) - def get_proc_exception(self) -> Exception: + def get_proc_exception(self, p: subprocess.Popen) -> Exception: """Returns an exception associated with a process exit code""" - assert self.proc - error_code = self.proc.wait(3) + error_code = p.wait(3) return errors.exception_from_error_code(error_code) @abstractmethod diff --git a/dangerzone/isolation_provider/qubes.py b/dangerzone/isolation_provider/qubes.py index 4c098f775..d77c13bc1 100644 --- a/dangerzone/isolation_provider/qubes.py +++ b/dangerzone/isolation_provider/qubes.py @@ -23,10 +23,6 @@ class Qubes(IsolationProvider): """Uses a disposable qube for performing the conversion""" - def __init__(self) -> None: - self.proc: Optional[subprocess.Popen] = None - super().__init__() - def install(self) -> bool: return True diff --git a/tests/isolation_provider/base.py b/tests/isolation_provider/base.py index d03a2b115..b9071b0f6 100644 --- a/tests/isolation_provider/base.py +++ b/tests/isolation_provider/base.py @@ -1,7 +1,9 @@ import os +import subprocess import pytest from colorama import Style +from pytest import MonkeyPatch from pytest_mock import MockerFixture from dangerzone.conversion import errors @@ -30,13 +32,25 @@ def test_max_pages_server_enforcement( pdf_11k_pages: str, provider: base.IsolationProvider, mocker: MockerFixture, + monkeypatch: MonkeyPatch, tmpdir: str, ) -> None: provider.progress_callback = mocker.MagicMock() doc = Document(pdf_11k_pages) - with pytest.raises(errors.ConverterProcException): + + proc = None + provider.old_start_doc_to_pixels_proc = provider.start_doc_to_pixels_proc # type: ignore [attr-defined] + + def start_doc_to_pixels_proc() -> subprocess.Popen: + proc = provider.old_start_doc_to_pixels_proc() # type: ignore [attr-defined] + return proc + + monkeypatch.setattr( + provider, "start_doc_to_pixels_proc", start_doc_to_pixels_proc + ) + with pytest.raises(errors.InterruptedConversionException): provider.doc_to_pixels(doc, tmpdir) - assert provider.get_proc_exception() == errors.MaxPagesException + assert provider.get_proc_exception(proc) == errors.MaxPagesException # type: ignore [arg-type] def test_max_pages_client_enforcement( self, diff --git a/tests/isolation_provider/test_qubes.py b/tests/isolation_provider/test_qubes.py index bf8b6afea..c21614a7b 100644 --- a/tests/isolation_provider/test_qubes.py +++ b/tests/isolation_provider/test_qubes.py @@ -40,8 +40,10 @@ def test_out_of_ram( ) -> None: provider.progress_callback = mocker.MagicMock() + proc = None + def start_doc_to_pixels_proc() -> subprocess.Popen: - p = subprocess.Popen( + proc = subprocess.Popen( # XXX error 126 simulates a qrexec-policy failure. Source: # https://github.com/QubesOS/qubes-core-qrexec/blob/fdcbfd7/daemon/qrexec-daemon.c#L1022 ["exit 126"], @@ -50,13 +52,13 @@ def start_doc_to_pixels_proc() -> subprocess.Popen: stderr=subprocess.PIPE, shell=True, ) - return p + return proc monkeypatch.setattr( provider, "start_doc_to_pixels_proc", start_doc_to_pixels_proc ) - with pytest.raises(errors.ConverterProcException) as e: + with pytest.raises(errors.InterruptedConversionException) as e: doc = Document(sample_doc) provider.doc_to_pixels(doc, tmpdir) - assert provider.get_proc_exception() == errors.QubesQrexecFailed + assert provider.get_proc_exception(proc) == errors.QubesQrexecFailed # type: ignore [arg-type]