From 673a7b4fe9b1f407ef9ed37ba5cd88cb408473ae Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:17:04 -0700 Subject: [PATCH 1/4] fix(sessions): Escape strings in the sessions __repr__ output The sessions 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`. The literal then does not parse at all: >>> repr(ActionResult(state=ActionState.SUCCESS, exit_code=0, ... stdout="a\xa0b")) 'ActionResult(state=SUCCESS, exit_code=Some(0), stdout="a\\u{a0}b")' SyntaxError: truncated \uXXXX escape 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 reach log lines and exception messages. Every string field now goes through CPython's own `repr()` via a new `py_repr` module. Delegating retires the bug class instead of reimplementing Python's escaping table: the output is by construction whatever the running interpreter produces, including its per-string choice of quote character. openjd-rs#374 added an escaping writer upstream for `ExprValue`, but it is `pub(crate)` in openjd-expr and so unreachable from this crate; PyO3 exposes the interpreter's `repr()` directly, which needs no upstream change and is the same oracle the tests assert against. Two neighbouring defects in the same expressions went with it, because leaving them would have kept the repr un-evaluable and so left the fix unverifiable by round-trip: * `Debug` for `Option` emitted `exit_code=Some(0)`, a `NameError`. * The state field rendered as a bare `Success` / `SUCCESS`, also a `NameError`. It now uses the spelling the enum's own repr already produces, `ActionState.SUCCESS`. `eval(repr(x)) == x` now holds for `ActionResult` across all 16 test inputs, which is what pins the escaping rather than a spelling assertion alone. Four sites changed: `ActionResult`, `ActionStatus`, `PosixSessionUser`, `WindowsSessionUser` and `Session`. `SessionState.__repr__` and `ScriptRunnerState.__repr__` were already correct. Adds test/openjd/sessions/, which did not exist -- the sessions surface had no Python tests beyond a module-name check. 113 cases over 16 inputs: the characters Rust renders as `\u{...}`, the ones it escapes correctly, printable non-ASCII that must survive verbatim, and negative controls for text needing no escaping. `group` is asserted separately from `user` because a fix applied to only the first argument would pass every `user` case. Mutation-checked, each behaviour reverted independently against a green 113-case baseline: escaping 67 failed, Option 44 failed, enum spelling 43 failed. No stub regeneration: a `Python<'_>` token is invisible to Python and `PyResult` still maps to `str`, so the emitted signature is unchanged. `SymbolTable.__repr__` already uses this form and its stub entry is `def __repr__(self) -> builtins.str`. WindowsSessionUser is unverified -- it cannot be constructed off Windows (`RuntimeError: Only available on Windows systems`), so its change is by inspection and shares the helper the other three sites test. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- rust-bindings/src/lib.rs | 1 + rust-bindings/src/py_repr.rs | 34 +++++ rust-bindings/src/sessions/session.rs | 8 +- rust-bindings/src/sessions/session_user.rs | 19 ++- rust-bindings/src/sessions/types.rs | 19 +-- test/openjd/sessions/__init__.py | 2 + test/openjd/sessions/test_repr.py | 168 +++++++++++++++++++++ 7 files changed, 232 insertions(+), 19 deletions(-) create mode 100644 rust-bindings/src/py_repr.rs create mode 100644 test/openjd/sessions/__init__.py create mode 100644 test/openjd/sessions/test_repr.py 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..da54a04b --- /dev/null +++ b/rust-bindings/src/py_repr.rs @@ -0,0 +1,34 @@ +// 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` happens to agree with Python on the quote, the backslash and the +//! C0 controls, but it renders anything else non-printable as `\u{a0}`, +//! and CPython wants exactly four hex digits after `\u`. A repr carrying +//! such a character does not parse at all, so a value that arrives from +//! outside -- captured process output, a template-supplied name -- can +//! corrupt the repr of the object holding it. +//! +//! Delegating to CPython's own `repr()` retires the bug class instead of +//! reimplementing its escaping table: the output is by construction +//! whatever the running interpreter produces, including its per-string +//! choice of quote character. + +use pyo3::prelude::*; +use pyo3::types::PyString; + +/// CPython's `repr()` of `value`, ready to embed in a `__repr__`. +pub(crate) fn py_str(py: Python<'_>, value: &str) -> PyResult { + Ok(PyString::new(py, value).repr()?.to_string()) +} + +/// 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..a282f16d 100644 --- a/rust-bindings/src/sessions/session.rs +++ b/rust-bindings/src/sessions/session.rs @@ -691,8 +691,12 @@ impl PySession { } } - fn __repr__(&self) -> String { + fn __repr__(&self, py: Python<'_>) -> PyResult { let snap = lock_recover(&self.snapshot); - format!("Session(id={:?}, state={:?})", snap.session_id, snap.state) + Ok(format!( + "Session(id={}, state=SessionState.{})", + crate::py_repr::py_str(py, &snap.session_id)?, + crate::sessions::types::PySessionState::from(snap.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..57f600c5 --- /dev/null +++ b/test/openjd/sessions/test_repr.py @@ -0,0 +1,168 @@ +# 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 pytest + +from openjd._openjd_rs import ( + ActionResult, + ActionState, + ActionStatus, + PosixSessionUser, +) + +# The characters Rust's Debug renders as \u{...}: non-printable outside +# ASCII (U+00A0, U+3000, U+0085) plus non-printable ASCII (U+007F). Then +# the ones Debug does escape correctly, kept as controls against a +# hand-rolled replacement getting them wrong, and printable non-ASCII +# that must survive verbatim. +HOSTILE_STRINGS = [ + "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") + + +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.""" + + @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)) + # Evaluates without NameError; ActionStatus has no __eq__ against a + # rebuilt instance's other fields, so this asserts evaluability only. + eval(repr(status), dict(EVAL_NS)) + + 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)" + ) + + +class TestPosixSessionUserRepr: + """``user`` and ``group`` arrive from outside.""" + + @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 TestReprNegativeControls: + """Text needing no escaping must pass through unaltered.""" + + 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')" + ) + + 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" From 4a464df7efe9f218da755112f1175a9be7f585ab Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:32:51 -0700 Subject: [PATCH 2/4] test(sessions): Gate the PosixSessionUser repr tests on posix The binding gates construction on `#[cfg(unix)]` and raises `RuntimeError: Only available on posix systems.` elsewhere, so the four tests that build one failed on the Windows matrix: 49 failures, exactly the 16+16+16+1 cases those tests parametrize. Mirrors the Rust guard with `os.name`. WindowsSessionUser gets no counterpart: 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. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- test/openjd/sessions/test_repr.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/test/openjd/sessions/test_repr.py b/test/openjd/sessions/test_repr.py index 57f600c5..d44cb742 100644 --- a/test/openjd/sessions/test_repr.py +++ b/test/openjd/sessions/test_repr.py @@ -21,6 +21,8 @@ from __future__ import annotations +import os + import pytest from openjd._openjd_rs import ( @@ -128,8 +130,16 @@ def test_repr_renders_none_exit_code(self) -> None: ) +@pytest.mark.skipif(os.name != "posix", reason="PosixSessionUser is constructible only on posix") class TestPosixSessionUserRepr: - """``user`` and ``group`` arrive from outside.""" + """``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: @@ -158,6 +168,9 @@ def test_plain_action_result(self) -> None: "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')" From 9a961e2257f21948c1e52ab8af494582c0dc2436 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Thu, 10 Sep 2026 19:17:00 -0700 Subject: [PATCH 3/4] fix(sessions): Address review findings on the repr escaping Five defects from review, each measured before fixing. **py_repr::py_str could panic instead of raising.** `.to_string()` on a `Bound` resolves to PyO3's `Display`, which calls `str()` on the object and has nowhere to put a `PyErr` -- `ToString` treats the formatter error as unreachable and panics. That put an unwind across the FFI boundary in the one function four reprs route through, and reprs are evaluated from logging and exception formatting where an unwind loses the diagnostic that prompted it. Now uses `to_cow()?`, which reads the UTF-8 directly, propagates failure, and skips a redundant interpreter round-trip. **Session's repr held the snapshot mutex across a CPython call.** `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 a non-reentrant `Mutex`. `lock_recover` maps a poisoned lock to `into_inner()`, so a panic there would not even surface. Reads `session_id` and `state` out from under the guard and drops it before formatting. Recorded in the module docs as a rule for the helper rather than a one-off. **Session's repr used `id=`, not the constructor's `session_id=`.** It parsed and then raised `TypeError: got an unexpected keyword argument 'id'` -- arguably worse than the old form, which failed at `compile()`. **PathMappingRule corrupted Windows paths silently.** Not a `{:?}` at all, so it was invisible to the search that found the others: it hand-rolled `'{}'` with no escaping. `C:\temp` rendered as `'C:\temp'`, which Python reads as `C:` + TAB + `emp` -- it parses and gives back a different string. An apostrophe in a path closed the literal early. This is the only repr found that corrupts on its most typical input, a Windows destination path, which is what path mapping exists to produce. Now routes through `py_str`. **ESC was missing from the test inputs.** Rust's `Debug` special-cases only the quote, the backslash, and NUL/tab/CR/LF; every other control falls through to the unparseable brace form. So ANSI colour sequences in captured stdout were almost certainly the highest-frequency trigger of this bug in the field, and no case exercised them. Adds ESC, U+0001 and U+001F, and narrows the module docs, which previously implied only non-ASCII was affected. `"a\x00b"` was also mislabelled as representative of C0 when it passes only because NUL has its own arm. Also corrects two claims of mine that were wrong: * A test comment said `ActionStatus` has no `__eq__`. It does (types.rs:337), comparing seven fields. The real reason its repr cannot round-trip is that the repr emits two of them, and it cannot be fixed by adding fields because `started_at`/`ended_at` are not constructor arguments. The misleading `eval()` assertion is replaced by one that pins the lossiness explicitly, so a later change to the field list is deliberate. * `py_repr`'s docs said delegating "retires the bug class". It retires it for `sessions/`; `model/` and the rest of `expr/` still carry it. Scoped the wording and pointed at the tracking note. Adds coverage for `Session` and `PathMappingRule`, which the first revision omitted -- both are constructible in a unit test, so the omissions were oversights rather than constraints. `Session` skips the NUL case: the id becomes a working-directory path component and the filesystem rejects it before any repr is taken, which is a constructor constraint, not a repr gap. 176 tests in the sessions module, up from 113. Two new mutants against a green baseline: reverting PathMappingRule to hand-rolled quoting fails 21, reverting Session's keyword fails 19. The pre-existing `TestPathMappingRuleRepr` in test/openjd/expr/test_path_mapping.py passes unmodified, which is the evidence that the change is a no-op for well-behaved paths. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- rust-bindings/src/expr/path_mapping.rs | 16 ++-- rust-bindings/src/py_repr.rs | 32 +++++-- rust-bindings/src/sessions/session.rs | 15 +++- test/openjd/sessions/test_repr.py | 119 +++++++++++++++++++++++-- 4 files changed, 155 insertions(+), 27 deletions(-) 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/py_repr.rs b/rust-bindings/src/py_repr.rs index da54a04b..e1c4148a 100644 --- a/rust-bindings/src/py_repr.rs +++ b/rust-bindings/src/py_repr.rs @@ -4,24 +4,38 @@ //! Rendering values into `__repr__` output that Python can parse. //! //! `format!("{:?}", s)` is not a Python literal writer. Rust's `Debug` for -//! `str` happens to agree with Python on the quote, the backslash and the -//! C0 controls, but it renders anything else non-printable as `\u{a0}`, -//! and CPython wants exactly four hex digits after `\u`. A repr carrying -//! such a character does not parse at all, so a value that arrives from -//! outside -- captured process output, a template-supplied name -- can -//! corrupt the repr of the object holding it. +//! `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()` retires the bug class instead of +//! 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; +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 { - Ok(PyString::new(py, value).repr()?.to_string()) + // `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)`, diff --git a/rust-bindings/src/sessions/session.rs b/rust-bindings/src/sessions/session.rs index a282f16d..9c13c034 100644 --- a/rust-bindings/src/sessions/session.rs +++ b/rust-bindings/src/sessions/session.rs @@ -692,11 +692,18 @@ impl PySession { } fn __repr__(&self, py: Python<'_>) -> PyResult { - let snap = lock_recover(&self.snapshot); + // 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(id={}, state=SessionState.{})", - crate::py_repr::py_str(py, &snap.session_id)?, - crate::sessions::types::PySessionState::from(snap.state).name() + "Session(session_id={}, state=SessionState.{})", + crate::py_repr::py_str(py, &session_id)?, + crate::sessions::types::PySessionState::from(state).name() )) } } diff --git a/test/openjd/sessions/test_repr.py b/test/openjd/sessions/test_repr.py index d44cb742..0fc36fe5 100644 --- a/test/openjd/sessions/test_repr.py +++ b/test/openjd/sessions/test_repr.py @@ -29,15 +29,25 @@ ActionResult, ActionState, ActionStatus, + PathFormat, + PathMappingRule, PosixSessionUser, + Session, + SessionState, ) -# The characters Rust's Debug renders as \u{...}: non-printable outside -# ASCII (U+00A0, U+3000, U+0085) plus non-printable ASCII (U+007F). Then -# the ones Debug does escape correctly, kept as controls against a -# hand-rolled replacement getting them wrong, and printable non-ASCII -# that must survive verbatim. +# 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", @@ -114,16 +124,35 @@ def test_repr_names_the_state_as_python(self, state: ActionState) -> None: class TestActionStatusRepr: - """No string field, but the same ``Option`` and enum defects.""" + """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)) - # Evaluates without NameError; ActionStatus has no __eq__ against a - # rebuilt instance's other fields, so this asserts evaluability only. + # 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)" @@ -160,8 +189,80 @@ def test_repr_matches_cpython_for_group(self, value: str) -> None: ) +class TestSessionRepr: + """``session_id`` is supplied by the caller, so it needs escaping too. + + A real ``Session`` creates a working directory, so each case calls + ``cleanup()``. The keyword must match the constructor: the repr used to + say ``id=``, which parsed but raised ``TypeError`` on eval. + """ + + @staticmethod + def _session(session_id: str) -> Session: + return Session(session_id=session_id, job_parameter_values={}) + + # `session_id` becomes a path component of the working directory, so NUL + # is refused by the filesystem before any repr is taken ("file name + # contained an unexpected NUL byte"). That is a constructor constraint, + # not a repr gap -- ActionResult covers NUL through the same helper. + SESSION_ID_CASES = [s for s in HOSTILE_STRINGS if "\x00" not in s] + + @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: - """Text needing no escaping must pass through unaltered.""" def test_plain_action_result(self) -> None: assert repr(ActionResult(state=ActionState.SUCCESS, exit_code=0, stdout="ok")) == ( From 1fb4d460e1bfb8a38d3728f3c577a95299cb945a Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Thu, 10 Sep 2026 19:30:08 -0700 Subject: [PATCH 4/4] test(sessions): Bound the Session repr cases by filename portability Windows CI failed 8 of the new TestSessionRepr cases: Session names its working directory after the session_id, so the inputs are bounded by what a filename may contain, not by what the repr can render. ESC, U+0001, U+001F, tab, newline, CRLF, the double quote and the backslash are all legal on APFS and ext4 and rejected by Windows -- 'The filename, directory name, or volume label syntax is incorrect. (os error 123)'. The previous revision excluded only NUL, which is the one case macOS caught. That fixed the instance rather than the class. Now filtered by predicate -- the C0 controls plus the punctuation Windows reserves -- so a new HOSTILE_STRINGS entry is classified automatically instead of silently breaking one platform. 10 of 19 cases survive, including U+00A0 and U+3000, the characters this change exists for. The 9 excluded are not left unverified: ActionResult and PosixSessionUser parametrize the full list through the same py_repr::py_str helper and touch no disk. What Session verifies is that it routes through that helper at all, and that its repr keyword matches its constructor. Adds test_the_case_filter_keeps_the_canonical_trigger so the filter cannot quietly empty out and leave the sweep asserting nothing. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- test/openjd/sessions/test_repr.py | 39 ++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/test/openjd/sessions/test_repr.py b/test/openjd/sessions/test_repr.py index 0fc36fe5..a07aba2e 100644 --- a/test/openjd/sessions/test_repr.py +++ b/test/openjd/sessions/test_repr.py @@ -87,6 +87,16 @@ def assert_parses(text: str) -> None: 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.""" @@ -192,20 +202,33 @@ def test_repr_matches_cpython_for_group(self, value: str) -> None: class TestSessionRepr: """``session_id`` is supplied by the caller, so it needs escaping too. - A real ``Session`` creates a working directory, so each case calls - ``cleanup()``. The keyword must match the constructor: the repr used to - say ``id=``, which parsed but raised ``TypeError`` on eval. + 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={}) - # `session_id` becomes a path component of the working directory, so NUL - # is refused by the filesystem before any repr is taken ("file name - # contained an unexpected NUL byte"). That is a constructor constraint, - # not a repr gap -- ActionResult covers NUL through the same helper. - SESSION_ID_CASES = [s for s in HOSTILE_STRINGS if "\x00" not in s] + 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: