diff --git a/rust-bindings/src/expr/path_mapping.rs b/rust-bindings/src/expr/path_mapping.rs index 78a421aa..77a8c6b0 100644 --- a/rust-bindings/src/expr/path_mapping.rs +++ b/rust-bindings/src/expr/path_mapping.rs @@ -122,16 +122,22 @@ impl PyPathMappingRule { &self.inner.destination_path } - fn __repr__(&self) -> String { + fn __repr__(&self, py: Python<'_>) -> PyResult { // Render `source_path_format` using its Python name // (`PathFormat.POSIX`) rather than the underlying Rust // enum's `Debug` name (`Posix`). Matches the Python // convention for enum repr. let fmt: PyPathFormat = self.inner.source_path_format.into(); - format!( - "PathMappingRule(source_path_format=PathFormat.{}, source_path='{}', destination_path='{}')", - fmt.variant_name(), self.inner.source_path, self.inner.destination_path - ) + // The paths go through CPython's repr rather than `'{}'`. Hand-rolled + // quoting corrupted a Windows destination silently: `C:\temp` emitted + // `'C:\temp'`, which Python reads as `C:` + TAB + `emp`, and an + // apostrophe in a path closed the literal early. + Ok(format!( + "PathMappingRule(source_path_format=PathFormat.{}, source_path={}, destination_path={})", + fmt.variant_name(), + crate::py_repr::py_str(py, &self.inner.source_path)?, + crate::py_repr::py_str(py, &self.inner.destination_path)?, + )) } /// Two `PathMappingRule`s compare equal when they have the diff --git a/rust-bindings/src/lib.rs b/rust-bindings/src/lib.rs index d20db372..b90f334f 100644 --- a/rust-bindings/src/lib.rs +++ b/rust-bindings/src/lib.rs @@ -4,6 +4,7 @@ mod expr; mod model; mod pickle_helpers; +mod py_repr; mod sessions; use pyo3::prelude::*; diff --git a/rust-bindings/src/py_repr.rs b/rust-bindings/src/py_repr.rs new file mode 100644 index 00000000..e1c4148a --- /dev/null +++ b/rust-bindings/src/py_repr.rs @@ -0,0 +1,48 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Rendering values into `__repr__` output that Python can parse. +//! +//! `format!("{:?}", s)` is not a Python literal writer. Rust's `Debug` for +//! `str` special-cases only the quote, the backslash, and NUL, tab, CR and +//! LF; every other control character and every non-printable falls through +//! to Rust's brace form, `\u{1b}` or `\u{a0}`. Python wants exactly four +//! hex digits after `\u`, so those do not parse. ESC is the one to keep in +//! mind: ANSI colour sequences in captured process output hit this far more +//! often than any exotic codepoint does. +//! +//! Delegating to CPython's own `repr()` removes the guesswork rather than +//! reimplementing its escaping table: the output is by construction +//! whatever the running interpreter produces, including its per-string +//! choice of quote character. +//! +//! Scope: the `sessions` reprs route through here. Reprs under `model/` +//! and the rest of `expr/` still use `{:?}` or hand-rolled quoting and +//! carry the same defect — see the tracking note in the pull request that +//! introduced this module. A new repr should use these helpers. +//! +//! Callers must not hold a lock across `py_str`: it re-enters the +//! interpreter, which can run arbitrary Python (allocation may trigger a +//! GC pass and with it `__del__` and weakref callbacks). Read what you +//! need out from under the guard, drop it, then format. + +use pyo3::prelude::*; +use pyo3::types::{PyString, PyStringMethods}; + +/// CPython's `repr()` of `value`, ready to embed in a `__repr__`. +pub(crate) fn py_str(py: Python<'_>, value: &str) -> PyResult { + // `to_cow` reads the UTF-8 directly and propagates failure. Going via + // `to_string()` would resolve to PyO3's `Display`, which calls `str()` + // on the object -- a second interpreter round-trip whose error has + // nowhere to go but a panic out of `__repr__`. + Ok(PyString::new(py, value).repr()?.to_cow()?.into_owned()) +} + +/// An optional `int` as Python spells it. `Debug` would emit `Some(0)`, +/// which evaluates to a `NameError`. +pub(crate) fn py_opt_int(value: Option) -> String { + match value { + Some(v) => v.to_string(), + None => "None".to_string(), + } +} diff --git a/rust-bindings/src/sessions/session.rs b/rust-bindings/src/sessions/session.rs index e569bdda..9c13c034 100644 --- a/rust-bindings/src/sessions/session.rs +++ b/rust-bindings/src/sessions/session.rs @@ -691,8 +691,19 @@ impl PySession { } } - fn __repr__(&self) -> String { - let snap = lock_recover(&self.snapshot); - format!("Session(id={:?}, state={:?})", snap.session_id, snap.state) + fn __repr__(&self, py: Python<'_>) -> PyResult { + // Read out from under the guard and drop it before calling into + // CPython: `py_str` allocates, an allocation can trigger a GC pass, + // and a finalizer run by that pass may re-enter this Session and + // re-lock `snapshot`, which is not reentrant. + let (session_id, state) = { + let snap = lock_recover(&self.snapshot); + (snap.session_id.clone(), snap.state) + }; + Ok(format!( + "Session(session_id={}, state=SessionState.{})", + crate::py_repr::py_str(py, &session_id)?, + crate::sessions::types::PySessionState::from(state).name() + )) } } diff --git a/rust-bindings/src/sessions/session_user.rs b/rust-bindings/src/sessions/session_user.rs index c41c4714..e1441f39 100644 --- a/rust-bindings/src/sessions/session_user.rs +++ b/rust-bindings/src/sessions/session_user.rs @@ -63,12 +63,12 @@ impl PyPosixSessionUser { self.inner.is_process_user() } - fn __repr__(&self) -> String { - format!( - "PosixSessionUser(user={:?}, group={:?})", - self.inner.user(), - self.inner.group() - ) + fn __repr__(&self, py: Python<'_>) -> PyResult { + Ok(format!( + "PosixSessionUser(user={}, group={})", + crate::py_repr::py_str(py, self.inner.user())?, + crate::py_repr::py_str(py, self.inner.group())? + )) } /// Pickle support — round-trips through `__init__(user, *, group=...)`. @@ -307,8 +307,11 @@ impl PyWindowsSessionUser { self.inner.is_process_user() } - fn __repr__(&self) -> String { - format!("WindowsSessionUser(user={:?})", self.inner.user()) + fn __repr__(&self, py: Python<'_>) -> PyResult { + Ok(format!( + "WindowsSessionUser(user={})", + crate::py_repr::py_str(py, self.inner.user())? + )) } /// Pickle support — round-trips through `__init__(user, *, diff --git a/rust-bindings/src/sessions/types.rs b/rust-bindings/src/sessions/types.rs index 9e2b7df7..450ace6a 100644 --- a/rust-bindings/src/sessions/types.rs +++ b/rust-bindings/src/sessions/types.rs @@ -53,7 +53,7 @@ impl From for PySessionState { impl PySessionState { /// Variant name as a string (e.g. `"READY"`). #[getter] - fn name(&self) -> &'static str { + pub(crate) fn name(&self) -> &'static str { match self { Self::READY => "READY", Self::RUNNING => "RUNNING", @@ -323,8 +323,9 @@ impl PyActionStatus { fn __repr__(&self) -> String { format!( - "ActionStatus(state={:?}, exit_code={:?})", - self.inner.state, self.inner.exit_code + "ActionStatus(state=ActionState.{}, exit_code={})", + self.state().name(), + crate::py_repr::py_opt_int(self.inner.exit_code) ) } @@ -525,13 +526,13 @@ impl PyActionResult { } } - fn __repr__(&self) -> String { - format!( - "ActionResult(state={}, exit_code={:?}, stdout={:?})", + fn __repr__(&self, py: Python<'_>) -> PyResult { + Ok(format!( + "ActionResult(state=ActionState.{}, exit_code={}, stdout={})", self.state.name(), - self.exit_code, - self.stdout, - ) + crate::py_repr::py_opt_int(self.exit_code), + crate::py_repr::py_str(py, &self.stdout)?, + )) } fn __eq__(&self, other: &Self) -> bool { diff --git a/test/openjd/sessions/__init__.py b/test/openjd/sessions/__init__.py new file mode 100644 index 00000000..04f8b7b7 --- /dev/null +++ b/test/openjd/sessions/__init__.py @@ -0,0 +1,2 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 diff --git a/test/openjd/sessions/test_repr.py b/test/openjd/sessions/test_repr.py new file mode 100644 index 00000000..a07aba2e --- /dev/null +++ b/test/openjd/sessions/test_repr.py @@ -0,0 +1,305 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""``__repr__`` output for the sessions bindings must be parseable Python. + +These reprs were built with ``format!("{:?}")``, which is not a Python +literal writer. Rust's ``Debug`` for ``str`` agrees with Python on the +quote, the backslash and the C0 controls, but renders anything else +non-printable as ``\\u{a0}`` -- and CPython wants exactly four hex digits +after ``\\u``, so the literal does not parse. ``Debug`` for ``Option`` +likewise emits ``Some(0)``, which is a ``NameError``. + +That made a repr corruptible by its own data. ``ActionResult.stdout`` is +captured process output, so a non-ASCII byte in a job's stdout is ordinary +rather than adversarial, and ``PosixSessionUser`` carries a user name that +arrives from outside. Both land in log lines and exception messages. + +Every string field now goes through CPython's own ``repr()``, so the +escaping is by construction whatever the running interpreter produces. +""" + +from __future__ import annotations + +import os + +import pytest + +from openjd._openjd_rs import ( + ActionResult, + ActionState, + ActionStatus, + PathFormat, + PathMappingRule, + PosixSessionUser, + Session, + SessionState, +) + +# The characters Rust's Debug renders in its brace form, which Python +# cannot parse. Debug special-cases only the quote, the backslash, and +# NUL/tab/CR/LF; every OTHER control falls through, so ESC is included +# deliberately -- ANSI colour sequences in captured stdout are the most +# likely trigger of this bug in the field, far more so than any exotic +# codepoint. Then the escapes Debug does get right, kept as controls +# against a hand-rolled replacement breaking them, and printable +# non-ASCII that must survive verbatim. +HOSTILE_STRINGS = [ + "esc\x1b[0m", + "a\x01b", + "a\x1fb", + "a\xa0b", + "a\u3000b", + "a\x85b", + "a\x7fb", + "a\u200bb", + "a\U00100000b", + 'a"b', + "it's", + "a\\b", + "a\nb", + "a\r\nb", + "a\tb", + "a\x00b", + "café", + "a\U0001f600b", + "", +] + +ALL_ACTION_STATES = [ + ActionState.RUNNING, + ActionState.SUCCESS, + ActionState.FAILED, + ActionState.CANCELED, + ActionState.TIMEOUT, +] + +EVAL_NS = { + "ActionResult": ActionResult, + "ActionState": ActionState, + "ActionStatus": ActionStatus, + "PosixSessionUser": PosixSessionUser, +} + + +def assert_parses(text: str) -> None: + """The repr must at least be syntactically valid Python.""" + compile(text, "", "eval") + + +# Characters no portable filename may contain: the C0 controls plus the +# punctuation Windows reserves. Used to bound the `Session` cases, whose +# session_id becomes a directory name on disk. +_WINDOWS_RESERVED_IN_FILENAMES = frozenset('<>:"/\\|?*') + + +def is_portable_filename(value: str) -> bool: + return not any(c in _WINDOWS_RESERVED_IN_FILENAMES or ord(c) < 0x20 for c in value) + + +class TestActionResultRepr: + """``ActionResult.stdout`` is captured process output.""" + + @pytest.mark.parametrize("stdout", HOSTILE_STRINGS) + def test_repr_parses(self, stdout: str) -> None: + assert_parses(repr(ActionResult(state=ActionState.SUCCESS, exit_code=0, stdout=stdout))) + + @pytest.mark.parametrize("stdout", HOSTILE_STRINGS) + def test_repr_embeds_cpython_repr_of_stdout(self, stdout: str) -> None: + result = ActionResult(state=ActionState.SUCCESS, exit_code=0, stdout=stdout) + assert repr(result) == ( + f"ActionResult(state=ActionState.SUCCESS, exit_code=0, stdout={stdout!r})" + ) + + @pytest.mark.parametrize("stdout", HOSTILE_STRINGS) + def test_repr_round_trips(self, stdout: str) -> None: + result = ActionResult(state=ActionState.SUCCESS, exit_code=0, stdout=stdout) + assert eval(repr(result), dict(EVAL_NS)) == result + + @pytest.mark.parametrize( + "exit_code,expected", + [(0, "exit_code=0"), (1, "exit_code=1"), (-9, "exit_code=-9"), (None, "exit_code=None")], + ) + def test_repr_renders_exit_code_as_python(self, exit_code: int | None, expected: str) -> None: + # Debug would emit `Some(0)`, which is a NameError. + assert expected in repr( + ActionResult(state=ActionState.SUCCESS, exit_code=exit_code, stdout="") + ) + + @pytest.mark.parametrize("state", ALL_ACTION_STATES) + def test_repr_names_the_state_as_python(self, state: ActionState) -> None: + result = ActionResult(state=state, exit_code=0, stdout="") + assert f"state={state!r}" in repr(result) + assert eval(repr(result), dict(EVAL_NS)) == result + + +class TestActionStatusRepr: + """No string field, but the same ``Option`` and enum defects. + + This repr is deliberately lossy: it shows ``state`` and ``exit_code``, + not the other five fields ``__eq__`` compares. So it is *evaluable* + but does not round-trip, and it cannot be made to -- ``started_at`` + and ``ended_at`` are not constructor arguments. Do not read the + Python-literal spelling as a round-trip guarantee; the lossiness is + pinned below so a later change to the field list is a deliberate one. + """ + + @pytest.mark.parametrize("exit_code", [0, 1, -9, None]) + def test_repr_is_evaluable(self, exit_code: int | None) -> None: + status = ActionStatus(state=ActionState.SUCCESS, exit_code=exit_code) + assert_parses(repr(status)) + # The defect this pins: `Some(0)` and a bare `SUCCESS` both raised + # NameError. Evaluability only -- see the class docstring. + eval(repr(status), dict(EVAL_NS)) + + def test_repr_omits_fields_that_eq_compares(self) -> None: + # Guards the class docstring's claim rather than asserting a + # round-trip that cannot hold. + status = ActionStatus( + state=ActionState.SUCCESS, exit_code=0, progress=50.0, status_message="halfway" + ) + assert repr(status) == "ActionStatus(state=ActionState.SUCCESS, exit_code=0)" + rebuilt = eval(repr(status), dict(EVAL_NS)) + assert rebuilt != status + assert rebuilt.progress is None and status.progress == 50.0 + + def test_repr_renders_none_exit_code(self) -> None: + assert repr(ActionStatus(state=ActionState.FAILED, exit_code=None)) == ( + "ActionStatus(state=ActionState.FAILED, exit_code=None)" + ) + + +@pytest.mark.skipif(os.name != "posix", reason="PosixSessionUser is constructible only on posix") +class TestPosixSessionUserRepr: + """``user`` and ``group`` arrive from outside. + + The binding gates construction on ``#[cfg(unix)]`` and raises + ``RuntimeError: Only available on posix systems.`` elsewhere, so these + mirror that with ``os.name``. ``WindowsSessionUser`` has no counterpart + here: off the process user it demands a password or a logon token, so + it cannot be built with an arbitrary name just to read its repr. + """ + + @pytest.mark.parametrize("value", HOSTILE_STRINGS) + def test_repr_parses_for_user(self, value: str) -> None: + assert_parses(repr(PosixSessionUser(user=value, group="g"))) + + @pytest.mark.parametrize("value", HOSTILE_STRINGS) + def test_repr_matches_cpython_for_user(self, value: str) -> None: + assert repr(PosixSessionUser(user=value, group="g")) == ( + f"PosixSessionUser(user={value!r}, group={'g'!r})" + ) + + @pytest.mark.parametrize("value", HOSTILE_STRINGS) + def test_repr_matches_cpython_for_group(self, value: str) -> None: + # `group` is a separate argument to the same writer; a fix applied + # to only the first would pass every `user` case above. + assert repr(PosixSessionUser(user="u", group=value)) == ( + f"PosixSessionUser(user={'u'!r}, group={value!r})" + ) + + +class TestSessionRepr: + """``session_id`` is supplied by the caller, so it needs escaping too. + + A real ``Session`` creates a working directory *named after the + session_id*, so the inputs here are bounded by what a filename may + contain, not by what the repr can render. Anything the most restrictive + supported filesystem rejects fails in the constructor, before a repr is + ever taken -- on Windows that is the C0 controls plus ``<>:"/\\|?*``. + + The excluded characters are not left unverified: they go through the + same ``py_repr::py_str`` helper via ``ActionResult`` and + ``PosixSessionUser`` above, which touch no disk. What is verified here + is that ``Session`` routes through that helper at all, and that the + keyword matches its constructor. + """ + + # Computed rather than hand-listed so a new HOSTILE_STRINGS entry is + # classified automatically instead of silently breaking Windows CI. + SESSION_ID_CASES = [s for s in HOSTILE_STRINGS if is_portable_filename(s)] + + @staticmethod + def _session(session_id: str) -> Session: + return Session(session_id=session_id, job_parameter_values={}) + + def test_the_case_filter_keeps_the_canonical_trigger(self) -> None: + # Guards against the filter quietly emptying out and the sweep below + # asserting nothing. U+00A0 is the character this PR exists for. + assert "a\xa0b" in self.SESSION_ID_CASES + assert "a\u3000b" in self.SESSION_ID_CASES + assert len(self.SESSION_ID_CASES) >= 8 + + @pytest.mark.parametrize("session_id", SESSION_ID_CASES) + def test_repr_matches_cpython_for_session_id(self, session_id: str) -> None: + session = self._session(session_id) + try: + assert repr(session) == ( + f"Session(session_id={session_id!r}, state=SessionState.READY)" + ) + assert_parses(repr(session)) + finally: + session.cleanup() + + def test_repr_uses_the_constructor_keyword(self) -> None: + # `id=` parsed but was not a real argument, so eval raised TypeError. + session = self._session("s1") + try: + r = repr(session) + assert "session_id=" in r and "(id=" not in r + # `job_parameter_values` is required and absent from the repr, so + # a full round-trip is not available; this pins the keyword only. + with pytest.raises(TypeError): + eval(r, {"Session": Session, "SessionState": SessionState}) + finally: + session.cleanup() + + +class TestPathMappingRuleRepr: + """Hand-rolled ``'{}'`` quoting corrupted Windows paths silently. + + ``C:\\temp`` rendered as ``'C:\\temp'``, which Python reads as ``C:`` + + TAB + ``emp`` -- it parses, and yields the wrong string. An apostrophe + in a path closed the literal early instead. + """ + + @pytest.mark.parametrize("path", HOSTILE_STRINGS + ["C:\\temp", "C:\\x", "/home/o'brien"]) + def test_repr_matches_cpython_for_both_paths(self, path: str) -> None: + rule = PathMappingRule( + source_path_format=PathFormat.POSIX, source_path=path, destination_path=path + ) + assert repr(rule) == ( + "PathMappingRule(source_path_format=PathFormat.POSIX, " + f"source_path={path!r}, destination_path={path!r})" + ) + + @pytest.mark.parametrize("path", ["C:\\temp", "C:\\users", "/home/o'brien/scenes"]) + def test_repr_round_trips_a_windows_path(self, path: str) -> None: + # The silent-corruption case: this used to parse and give back a + # different string. + rule = PathMappingRule( + source_path_format=PathFormat.WINDOWS, source_path="/mnt/s", destination_path=path + ) + rebuilt = eval(repr(rule), {"PathMappingRule": PathMappingRule, "PathFormat": PathFormat}) + assert rebuilt.destination_path == path + assert rebuilt == rule + + +class TestReprNegativeControls: + + def test_plain_action_result(self) -> None: + assert repr(ActionResult(state=ActionState.SUCCESS, exit_code=0, stdout="ok")) == ( + "ActionResult(state=ActionState.SUCCESS, exit_code=0, stdout='ok')" + ) + + @pytest.mark.skipif( + os.name != "posix", reason="PosixSessionUser is constructible only on posix" + ) + def test_plain_posix_session_user(self) -> None: + assert repr(PosixSessionUser(user="alice", group="staff")) == ( + "PosixSessionUser(user='alice', group='staff')" + ) + + def test_state_enum_repr_unchanged(self) -> None: + # The spelling the reprs above embed. + assert repr(ActionState.SUCCESS) == "ActionState.SUCCESS"