fix(sessions): escape undecodable subprocess output bytes - #298
Merged
Conversation
The stdout read loop decoded each line with `String::from_utf8_lossy`, which replaces every undecodable byte with U+FFFD. The reader survives invalid input, but the original byte values are destroyed, so a session log gives no indication of what the subprocess actually emitted. The Python implementation decodes the same output with `errors="backslashreplace"`, so the two implementations logged different text for identical child output. Add `decode_backslashreplace`, which escapes every byte that is not valid UTF-8 as `\xNN` with lowercase hex, and use it in the stdout read loop. Preserving the byte values helps identify the code page a DCC application is emitting: the motivating case is a Windows fleet where Unreal Engine wrote the cp1252 em dash `0x97`, which previously appeared in the log as a bare replacement character. The implementation uses `slice::utf8_chunks` (stable since 1.79, below the 1.94.1 MSRV), so valid text needs no re-validation and invalid sequences are delimited exactly as UTF-8 validation defines them. Fully valid lines return `Cow::Borrowed` and do not allocate, which is the common case for the once-per-line hot path. Parity with CPython is pinned by a conformance table whose expected values were generated by CPython's own decoder, covering the customer's 0x97 byte, always-invalid bytes, consecutive invalid bytes, cp1252 text runs, truncated sequences both mid-line and at EOF, lone continuation bytes, encoded surrogates, overlong encodings, out-of-range lead bytes, and valid 2/3/4-byte sequences passing through unmodified. The algorithm was additionally differential-tested against CPython over 20,000 random and adversarial inputs with no divergence. Every test was mutation-checked. Reverting to `from_utf8_lossy`, uppercasing the hex digits, dropping invalid bytes, emitting U+FFFD, swapping the nibble order, dropping the valid prefix, removing the no-allocation fast path, and substituting a naive escape-all-non-ASCII decoder each fail at least one test. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Contributor
Author
|
Cross-reference: the Python-side fix this achieves parity with is openjd-sessions-for-python#343. Both originate from the same customer report. |
1 task
mwiebe
approved these changes
Aug 3, 2026
crowecawcaw
approved these changes
Aug 4, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #296
What was the problem/requirement? (What/Why)
The stdout read loop in
run_subprocessdecoded each line withString::from_utf8_lossy, which replaces every undecodable byte with U+FFFD. The reader survives invalid input (unlike the Python implementation, which had a genuine crash here), but the original byte values are destroyed, so the session log gives no indication of what the subprocess actually emitted.openjd-sessions-for-pythondecodes the same output witherrors="backslashreplace", so the two implementations logged different text for identical child output. That divergence is the subject of #296.The motivating case is a production Windows customer-managed fleet where Unreal Engine wrote the cp1252 em dash
0x97to stdout. In Python that byte crashed the stdout reader thread and lost all subsequent output (fixed in openjd-sessions-for-python#343, which chosebackslashreplaceso the byte value survives into the log). Here the same byte was silently flattened to�, leaving an operator no way to tell which code page the renderer was using.What was the solution? (How)
Add
decode_backslashreplace, which decodes as UTF-8 and escapes every byte that is not valid UTF-8 as\xNNwith lowercase hex, and use it in the stdout read loop.It is built on
slice::utf8_chunks(stable since 1.79.0, comfortably below the 1.94.1 MSRV), which yields (valid prefix, invalid sequence) pairs. Valid text therefore needs no re-validation, and invalid sequences are delimited exactly as UTF-8 validation defines them. CPython escapes each byte of an undecodable sequence individually, so a 3-byte invalid sequence such as an encoded surrogate produces three escapes; this matches that.Fully valid input returns
Cow::Borrowedand does not allocate. This matters because the function runs once per line of subprocess output, and it is a small improvement to the allocation chain flagged as item 4 inreports/sessions-quality-evaluation-report.md(that item remains open; this only removes the decode-step allocation on the valid path).What is the impact of this change?
�, so the emitting code page is identifiable.�now renders as\x97.from_utf8_lossyexpanded an invalid sequence to 3 bytes. A line consisting largely of undecodable bytes can therefore grow up to fourfold before the existing 64KB truncation is applied, rather than threefold. Both are bounded by the same truncation.How was this change tested?
Yes, unit tests were run:
cargo test -p openjd-sessionsis green (443 tests), as iscargo test -p openjd-cli(155 tests, the dependent crate).cargo clippy --all-features --all-targets --workspace -- -D warnings,cargo fmt --all --check,RUSTDOCFLAGS="-D warnings" cargo doc, and the copyright-header check all pass.Test-first: the integration test was written before the fix and fails against
from_utf8_lossywith"bad \u{fffd} byte!\n".New tests:
test_run_subprocess_invalid_utf8_is_escaped— end-to-end throughrun_subprocess: the customer's0x97byte appears as\x97and no U+FFFD is present.test_run_subprocess_valid_utf8_not_escaped— negative control against over-escaping, end to end.test_decode_backslashreplace_matches_cpython— a 17-case conformance table whose expected values were generated by CPython's own decoder, covering the0x97byte, always-invalid bytes, consecutive invalid bytes, cp1252 text runs, a truncated sequence mid-line and one at EOF (whereUtf8Error::error_len()isNone), a lone continuation byte, an encoded surrogate, an overlong encoding, an out-of-range lead byte, valid 2/3/4-byte sequences, empty input, an already-present literal backslash, and NUL/tab passthrough.test_decode_backslashreplace_borrows_valid_input— pins the no-allocation fast path.test_decode_backslashreplace_escapes_use_lowercase_hex— CPython emits lowercase; uppercase would be a visible divergence.Beyond the table, the algorithm was differential-tested against CPython over 20,000 random and adversarial inputs (uniform random bytes, sequences biased toward UTF-8 lead/continuation bytes, valid text with invalid bytes spliced in, and truncated valid text) with zero divergences.
Every test was mutation-checked; each of these fails at least one test:
from_utf8_lossy0x97would render\x79)The last mutant exists to prove the valid-UTF-8 negative controls are not vacuous: no other mutant makes them fail, because this implementation structurally cannot over-escape.
Not verified locally: the full
cargo test --workspace(theopenjd-snapshotsAWS SDK build exhausted disk on my machine) and the conformance suite. Both run in CI. Nothing in this change is reachable fromopenjd-snapshots, which does not depend onopenjd-sessions.Was this change documented?
Yes.
decode_backslashreplacecarries a doc comment explaining the failure mode, the Python parity requirement, and the borrow behavior. Per the spec co-evolution convention,specs/sessions/subprocess.mdis updated in the same commit: the stdout-loop excerpt now shows the new call, and a new "Decoding Undecodable Output Bytes" section documents the escaping, the CPython parity, and the fourfold expansion bound. The stalefrom_utf8_lossyreference in item 4 of the sessions quality report is corrected (the item itself stays open).No
public-api.mdchange:decode_backslashreplaceispub(crate), matching the neighbouringtruncate_line.Is this a breaking change?
No. No public API changes. The only observable difference is the text of logged subprocess output for processes that emit non-UTF-8 bytes.
Does this change impact security?
No new files, directories, or permissions, and no threat model change. Log content now includes escaped byte values in place of replacement characters, derived from the same subprocess output that was already being logged. If anything this is a small improvement: the escaped form is pure ASCII, so raw undecodable bytes are not passed through to the log sink.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.