diff --git a/.github/workflows/docker-pr.yml b/.github/workflows/docker-pr.yml index 019f59e..61d8c34 100644 --- a/.github/workflows/docker-pr.yml +++ b/.github/workflows/docker-pr.yml @@ -436,3 +436,51 @@ jobs: assert out.read_bytes()[:4] == b"%PDF", "no PDF produced" print("ok: gs stub shadows the real gs for soffice; engine=%s" % conv.engine_used) PY + + # soffice only launches LibreOffice: the conversion runs in a child of + # the launcher (soffice.bin). Past the time limit the DOCX converter + # kills the whole process group (app/converters/document.py::_run_soffice). + # Verify in the real image, with a document too long to finish in time, + # that the worker ran mid-conversion and is gone after the limit. + - name: Check a LibreOffice timeout stops all of LibreOffice + if: matrix.target == 'office' + run: | + docker exec -i filemorph python - <<'PY' + import subprocess, tempfile, threading, time + from pathlib import Path + from docx import Document + from app.converters.document import _convert_via_libreoffice + + def workers(): + """Running soffice.bin processes: (pid, parent pid, process group).""" + found = [] + for stat in Path("/proc").glob("[0-9]*/stat"): + try: + text = stat.read_text() + except OSError: + continue + state, ppid, pgid = text[text.rindex(")") + 2 :].split()[:3] + if text[text.index("(") + 1 : text.rindex(")")] == "soffice.bin" and state != "Z": + found.append((int(stat.parent.name), int(ppid), int(pgid))) + return found + + work = Path(tempfile.mkdtemp()) + doc = Document() + for _ in range(10000): + doc.add_paragraph("FileMorph time limit probe. " * 16) + doc.save(work / "long.docx") + mid = [] + threading.Timer(2, lambda: mid.extend(workers())).start() + try: + _convert_via_libreoffice(work / "long.docx", work / "out.pdf", timeout_s=5) + except subprocess.TimeoutExpired: + pass + else: + raise SystemExit("LibreOffice finished within 5 s; the check needs a longer document") + assert mid, "no LibreOffice worker ran mid-conversion; the check proves nothing" + deadline = time.monotonic() + 5 + while workers() and time.monotonic() < deadline: + time.sleep(0.1) + assert not workers(), f"LibreOffice still running after its time limit: {workers()}" + print(f"ok: worker {mid} (pid, parent pid, group) stopped at the time limit") + PY diff --git a/app/converters/document.py b/app/converters/document.py index 34ffa4e..3bb371a 100644 --- a/app/converters/document.py +++ b/app/converters/document.py @@ -6,6 +6,7 @@ import os import re import shutil +import signal import subprocess import zipfile from pathlib import Path @@ -238,6 +239,37 @@ def _soffice_child_env(shim_dir: Path, scratch_dir: Path) -> dict[str, str]: return env +def _run_soffice( + args: list[str], *, timeout: float, env: dict[str, str] +) -> subprocess.CompletedProcess: + """``subprocess.run(args, capture_output=True, timeout=timeout, env=env)`` + for ``soffice``, except that past ``timeout`` all of LibreOffice stops. + + ``soffice`` is a launcher: the conversion runs in LibreOffice's worker + (``soffice.bin``), a child of the launcher. Past its timeout, + ``subprocess.run`` stops only the program it started, which would leave + the worker running. Started in a session of its own, the call is one + process group, and past the timeout — or on any other error while + waiting — the whole group is killed. + """ + with subprocess.Popen( # noqa: S603 — binary resolved via shutil.which, no shell + args, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env=env, + start_new_session=True, + ) as proc: + try: + stdout, stderr = proc.communicate(timeout=timeout) + except BaseException: + if os.name == "posix": + os.killpg(proc.pid, signal.SIGKILL) + else: # pragma: no cover - os.killpg is POSIX-only + proc.kill() + raise + return subprocess.CompletedProcess(args, proc.returncode, stdout, stderr) + + def _convert_via_libreoffice(input_path: Path, output_path: Path, timeout_s: int) -> None: """Render DOCX → PDF using LibreOffice headless. @@ -265,7 +297,7 @@ def _convert_via_libreoffice(input_path: Path, output_path: Path, timeout_s: int # parallel instances. The user-profile directory is per-call so two # concurrent conversions don't clobber each other's lockfiles. profile_uri = (work_dir / "userprofile").resolve().as_uri() - result = subprocess.run( # noqa: S603 — binary resolved via shutil.which, no shell + result = _run_soffice( [ binary, "--headless", @@ -279,9 +311,7 @@ def _convert_via_libreoffice(input_path: Path, output_path: Path, timeout_s: int str(work_dir), str(input_path), ], - capture_output=True, timeout=timeout_s, - check=False, # Reduced child env: app secrets dropped, gs stubs ahead of the # real Ghostscript on PATH. See _soffice_child_env. env=_soffice_child_env(shim_dir, scratch_dir), diff --git a/changelog.d/2026-10-29-libreoffice-timeout.md b/changelog.d/2026-10-29-libreoffice-timeout.md new file mode 100644 index 0000000..eef7cdc --- /dev/null +++ b/changelog.d/2026-10-29-libreoffice-timeout.md @@ -0,0 +1,7 @@ +### Fixed — a LibreOffice time limit stops all of LibreOffice + +When a DOCX → PDF conversion through LibreOffice (office image) — including a +Word file inserted into a PDF — reaches `OFFICE_SUBPROCESS_TIMEOUT_SECONDS`, +the converter now stops LibreOffice's worker process (`soffice.bin`) together +with the `soffice` launcher, so the conversion ends completely at the limit. +For this, `soffice` runs in a process group of its own. diff --git a/docs/architecture.md b/docs/architecture.md index b3cc987..b479653 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -96,7 +96,9 @@ Every conversion or compression request follows the same path: `MEDIA_SUBPROCESS_TIMEOUT_SECONDS` (default 85 s for all of a request's audio and video work; video → GIF at most 85 s for all its steps together), LibreOffice `OFFICE_SUBPROCESS_TIMEOUT_SECONDS` (default - 60 s), Ghostscript 60 s. On Linux ffmpeg and ffprobe start through + 60 s), Ghostscript 60 s. On Linux LibreOffice runs in a process group + of its own, so its limit stops the worker (`soffice.bin`) along with + the `soffice` launcher. On Linux ffmpeg and ffprobe start through `app/converters/_limited_exec.py`, capped at 3 GiB of address space and at the request's output cap per file written. PDF → TXT runs pypdf in a child Python process diff --git a/docs/security-overview.md b/docs/security-overview.md index df8d2df..a09fb37 100644 --- a/docs/security-overview.md +++ b/docs/security-overview.md @@ -300,14 +300,20 @@ keeps working, as does the pure-Python (mammoth) path. This is in the same spirit as the Pillow setting that keeps WeasyPrint from reaching Ghostscript (`app/core/image_hardening.py`). +Past its time limit (`OFFICE_SUBPROCESS_TIMEOUT_SECONDS`), the subprocess is +stopped as a whole: `soffice` starts in a process group of its own, so +LibreOffice's worker process (`soffice.bin`) is killed along with the launcher +(`_run_soffice` in `app/converters/document.py`). + The PDF/A re-render runs only on an upload whose first non-blank bytes are the `%PDF-` header (`_has_pdf_header` in `app/converters/pdfa.py`); any other upload is refused with a caller-safe `400` before that stage, so only files that present as PDFs reach Ghostscript. -`tests/test_office_gs_isolation.py` pins the subprocess environment, and -`.github/workflows/docker-pr.yml` exercises a DOCX→PDF conversion on the office -image. +`tests/test_office_gs_isolation.py` pins the subprocess environment, +`tests/test_office_timeout.py` the stop at the time limit, and +`.github/workflows/docker-pr.yml` exercises a DOCX→PDF conversion and a time +limit on the office image. ### Decompression bombs diff --git a/docs/self-hosting.md b/docs/self-hosting.md index 8cc03d5..9628f80 100644 --- a/docs/self-hosting.md +++ b/docs/self-hosting.md @@ -159,9 +159,10 @@ fallback — it makes a missing `soffice` fail loud instead of silently degrading). `OFFICE_SUBPROCESS_TIMEOUT_SECONDS` (default `60`) bounds how long a -single `soffice --convert-to` call may run before it is killed — -raise it if you regularly convert long, complex Word documents on a -slower host. Ignored when `FILEMORPH_OFFICE_ENGINE=mammoth`. +single `soffice --convert-to` call may run before it is killed, together +with LibreOffice's worker process (`soffice.bin`) — raise it if you +regularly convert long, complex Word documents on a slower host. Ignored +when `FILEMORPH_OFFICE_ENGINE=mammoth`. ### Verifying signatures diff --git a/tests/test_log_privacy.py b/tests/test_log_privacy.py index 1f77c0a..308e765 100644 --- a/tests/test_log_privacy.py +++ b/tests/test_log_privacy.py @@ -443,6 +443,7 @@ def test_tool_errors_quote_stderr_only_with_app_debug(monkeypatch, tmp_path, deb monkeypatch.setattr(settings, "app_debug", debug) monkeypatch.setattr(subprocess, "run", _failed_run) + monkeypatch.setattr(document, "_run_soffice", _failed_run) monkeypatch.setattr(shutil, "which", lambda name: f"/usr/bin/{name}") monkeypatch.setattr(_ghostscript, "_GS_BINARY", "gs") errors = [] @@ -459,8 +460,8 @@ def test_tool_errors_quote_stderr_only_with_app_debug(monkeypatch, tmp_path, deb assert all("exit" in error or "status" in error for error in errors), errors # soffice exits 0 but writes no PDF: the same rule for its stderr. monkeypatch.setattr( - subprocess, - "run", + document, + "_run_soffice", lambda *a, **k: subprocess.CompletedProcess(a, 0, b"", b"SECRET-METADATA"), ) with pytest.raises(RuntimeError) as excinfo: diff --git a/tests/test_office_gs_isolation.py b/tests/test_office_gs_isolation.py index 046d61e..75daf97 100644 --- a/tests/test_office_gs_isolation.py +++ b/tests/test_office_gs_isolation.py @@ -9,8 +9,9 @@ keep working, with the app's secrets dropped from that gs as well; the pure-Python (mammoth) path is unaffected. -These tests fake ``shutil.which``/``subprocess.run`` so the wiring is asserted -on any platform, with no real ``soffice`` or ``gs`` installed. The real office +These tests fake ``shutil.which`` and the calls that start ``soffice``/``gs`` +(``_run_soffice``, ``subprocess.run``) so the wiring is asserted on any +platform, with no real ``soffice`` or ``gs`` installed. The real office image is exercised end-to-end in ``.github/workflows/docker-pr.yml``. """ @@ -20,6 +21,7 @@ import shutil import stat import subprocess +import sys from pathlib import Path from app.converters import _ghostscript, document @@ -95,8 +97,8 @@ def test_child_env_prepends_shims_and_drops_secrets(monkeypatch, tmp_path): def test_convert_via_libreoffice_runs_soffice_with_hardened_env(monkeypatch, tmp_path): """The routing guard the hardening turns on: ``soffice`` is invoked with a - Ghostscript stub ahead of any real gs, app secrets stripped, documented - flags, and no shell.""" + Ghostscript stub ahead of any real gs, app secrets stripped, and + documented flags.""" real_gs_dir = str(tmp_path / "usr-bin") fake_soffice = str(tmp_path / "soffice") monkeypatch.setenv("PATH", os.pathsep.join([str(tmp_path / "venv"), real_gs_dir])) @@ -127,7 +129,7 @@ def fake_run(argv, **kwargs): (outdir / f"{Path(argv[-1]).stem}.pdf").write_bytes(b"%PDF-1.7\n") return subprocess.CompletedProcess(argv, 0, stdout=b"", stderr=b"") - monkeypatch.setattr(document.subprocess, "run", fake_run) + monkeypatch.setattr(document, "_run_soffice", fake_run) src = tmp_path / "in.docx" src.write_bytes(b"PK\x03\x04") # the fake runner never reads it @@ -143,10 +145,7 @@ def fake_run(argv, **kwargs): assert "pdf:writer_pdf_Export" in argv kwargs = calls["kwargs"] - assert kwargs["check"] is False - assert kwargs["capture_output"] is True assert kwargs["timeout"] == 42 - assert kwargs.get("shell", False) is False # run() defaults to no shell env = kwargs["env"] assert "STRIPE_SECRET_KEY" not in env # secret stripped from the child @@ -157,6 +156,16 @@ def fake_run(argv, **kwargs): assert os.environ["STRIPE_SECRET_KEY"] == "sk_live_do_not_leak" +def test_run_soffice_hands_over_the_env_and_returns_like_subprocess_run(): + """``_run_soffice`` starts the program with exactly the env it is given and + returns its status and captured output (soffice's stderr can quote the + document) the way ``subprocess.run(capture_output=True)`` would.""" + code = "import os, sys; sys.stdout.write(os.environ['FM_PROBE']); sys.stderr.write('e'); sys.exit(3)" + env = {**os.environ, "FM_PROBE": "env reached the child"} + result = document._run_soffice([sys.executable, "-c", code], timeout=30, env=env) + assert (result.returncode, result.stdout, result.stderr) == (3, b"env reached the child", b"e") + + def test_the_pdfa_rerender_runs_ghostscript_without_secrets(monkeypatch, tmp_path): """FileMorph's own Ghostscript call gets the app's environment without its secrets too, with ``PATH`` as it is: no stub there, it runs the real gs.""" diff --git a/tests/test_office_timeout.py b/tests/test_office_timeout.py new file mode 100644 index 0000000..7f6e4c2 --- /dev/null +++ b/tests/test_office_timeout.py @@ -0,0 +1,76 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +"""Past its time limit, a LibreOffice conversion stops all of LibreOffice. + +``soffice`` is a launcher; the conversion runs in a child of it +(``soffice.bin``). A stand-in launcher of the same shape — a shell script that +starts a worker and waits for it — shows that stopping only the launcher, as +``subprocess.run`` does past its timeout, leaves the worker running, and that +``_convert_via_libreoffice`` stops the worker as well. The real LibreOffice is +checked the same way in ``.github/workflows/docker-pr.yml``. +""" + +from __future__ import annotations + +import os +import signal +import subprocess +import sys +import time +from pathlib import Path + +import pytest + +from app.converters import document + +pytestmark = pytest.mark.skipif(sys.platform != "linux", reason="reads processes from /proc") + + +def _stand_in_soffice(tmp_path: Path) -> tuple[Path, Path]: + """A launcher that starts a worker, writes the worker's PID to a file and + waits for it. Returns the launcher and the PID file.""" + pid_file = tmp_path / "worker.pid" + launcher = tmp_path / "soffice" + launcher.write_text( + f'#!/bin/sh\nsleep 60 &\necho $! > "{pid_file}"\nwait\n', encoding="utf-8", newline="\n" + ) + launcher.chmod(0o755) + return launcher, pid_file + + +def _running(pid: int) -> bool: + """Whether ``pid`` runs; a process that ended but is not reaped yet counts as ended.""" + try: + stat = Path(f"/proc/{pid}/stat").read_text() + except OSError: + return False + return stat.rsplit(")", 1)[1].split()[0] != "Z" + + +def test_a_plain_timeout_leaves_the_worker_running(tmp_path): + # The case the fix is for, and proof that the stand-in shows it: without + # this, the next test would also pass with a worker that ends on its own. + launcher, pid_file = _stand_in_soffice(tmp_path) + with pytest.raises(subprocess.TimeoutExpired): + subprocess.run([str(launcher)], capture_output=True, timeout=1, check=False) + worker = int(pid_file.read_text()) + alive = _running(worker) + if alive: + os.kill(worker, signal.SIGKILL) + assert alive + + +def test_a_libreoffice_timeout_stops_the_worker_too(monkeypatch, tmp_path): + launcher, pid_file = _stand_in_soffice(tmp_path) + monkeypatch.setattr(document.shutil, "which", lambda name: str(launcher)) + src = tmp_path / "in.docx" + src.write_bytes(b"PK\x03\x04") # the stand-in never reads it + with pytest.raises(subprocess.TimeoutExpired): + document._convert_via_libreoffice(src, tmp_path / "out.pdf", timeout_s=1) + worker = int(pid_file.read_text()) + deadline = time.monotonic() + 5 + while _running(worker) and time.monotonic() < deadline: + time.sleep(0.05) + alive = _running(worker) + if alive: + os.kill(worker, signal.SIGKILL) # not left behind when the test fails + assert not alive