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

@@ -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"

dangerzone/isolation_provider/base.py

@@ -27,15 +27,15 @@ 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
 
 
 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,16 +113,16 @@ 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):
                     raise errors.MaxPageHeightException()
 
                 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

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
 

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,

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]