fix(graphics)!: decode RFX Progressive SRL streams as Windows encodes them

[AKolenda]

Windows RFX Progressive upgrade streams may omit the final zero byte and trailing zero entries, or encode zero runs longer than the remaining coefficients. The decoder now accepts those streams without ending the session.

All bytes remain available until the requested coefficients are decoded. A final zero byte can contain sign or magnitude bits, so it is never stripped in advance. Zero-run events are consumed incrementally, and a pending nonzero coefficient is preserved even when its remaining bits come from the zero-filled reader at EOF. This matches FreeRDP's SRL reader; its optional trailing-byte skip happens after decoding.

The encoder is unchanged. Invalid magnitude widths still fail the upgrade pass without partially updating the tile. The decoder cannot distinguish omitted trailing entries from truncation, and the documentation now states that limitation.

Breaking changes

• Remove SrlError::MissingTerminator and SrlError::Truncated.
• SrlDecoder::new returns Self because constructing a decoder is infallible.

Validation

• All 245 ironrdp-graphics library tests pass, including new zero-byte/EOF boundary and round-trip regressions with and without terminators.
• cargo clippy --locked -p ironrdp-graphics --all-targets -- -D warnings
• Formatting and git diff --check pass.

Part of the Windows interoperability series #2007–#2017.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[AKolenda]

Related: #1977 also changes the SRL decoder, in a different way. Both stop requiring the terminator, read past the end as zeros, and remove MissingTerminator and Truncated. #1977 decodes the final byte as data and consumes zero runs one event at a time without a cap. This PR strips a trailing zero byte when present, as FreeRDP's progressive_rfx_upgrade_state_finish does, and caps a run at one component. They conflict in srl.rs and progressive.rs, so only one of them should land.

[meanaverage (meanaverage)]

Another data point for this one: we hit the same failure independently, with Windows 11 over the graphics pipeline through the web client (ironrdp-web). Upgrade passes failed first with the missing trailing zero byte and then, with that check removed, as truncated, and the session ended the first time Windows refined a picture. In our downstream build, not requiring the terminator and reading zero-run code words one at a time as values are needed (as FreeRDP does) fixed it.

I haven't run this branch itself, but the cases it handles cover what we saw. We'll drop our local patch once this or #1977 lands, so we're not opening a competing PR.

[github-actions]

PR #2010 relaxes the RFX Progressive SRL decoder to accept three Windows constructions: an optional trailing zero byte, omitted trailing entries (bits past end read as zeros), and zero runs overshooting one component (capped at 4096 instead of rejected). The progressive.rs changes are test-only. The leniency is protocol-permissible and matches FreeRDP behavior, the interop motivation is credible with failing-session reports, and the tile-untouched-on-error property is preserved. All six candidates are valid: one skeptical finding about the trailing-byte strip is refined because its concrete example is arithmetically wrong, though a corrected construction confirms the underlying ambiguity; the remaining skeptical findings (silent truncation, stale doc) and all five code-compressor dead-plumbing findings are accepted as low-severity. No correctness defect in the main decode paths was found.

1. [skeptical] Stripping a trailing 0x00 byte can drop a final positive max-magnitude value — medium 🟠 ❓ — crates/ironrdp-graphics/src/srl.rs
   new() removes any trailing 0x00, so a terminator-less stream whose final data byte is all zeros is indistinguishable from one carrying the terminator. Bits in a stripped byte read identically to past-end zeros, so the divergence is confined to nonzero_pending = !is_exhausted() (line 107): when a zero-run codeword ends exactly at the stripped-payload boundary, a following positive max-magnitude value whose sign and unary zeros formed the stripped byte is decoded as 0 instead. Example: data [0x86, 0x00] decoded for 3 entries gives [3, 0, 0] stripped versus [3, 15, 0] with the byte kept; the doc comment's claim that a stripped zero data byte is harmless is therefore overstated. Whether Windows can emit this shape is unverifiable from the repository.
2. [skeptical] Arbitrary mid-stream truncation now decodes silently with no signal — low 🟡 — crates/ironrdp-graphics/src/srl.rs
   The tolerance is justified for Windows omitting trailing entries, but is_exhausted() cannot distinguish an intentional early stop from corruption: truncation anywhere yields zeros, or a spurious positive maximum if a value was pending. This drops the malformed-stream detection half of #1696's guarantee while keeping only tile atomicity, so a transport bit error that ended the session now produces silent visual corruption with no log or counter. A trace/debug signal when the exhausted branch or the MAX_ZERO_RUN cap engages would partially restore observability at negligible cost.
3. [skeptical] decode_upgrade_pass doc still promises rejection of truncated streams — low 🟡 — crates/ironrdp-graphics/src/progressive.rs
   The Errors section at line 168 says the function returns SrlError for a malformed or truncated SRL stream, but after this PR truncation decodes as zeros or positive maxima and succeeds, mutating the tile. A reader would wrongly assume truncation still fails the pass. One-line doc fix on a changed path whose central behavioral claim it contradicts.
4. [code-compressor] SrlDecoder::new can no longer fail; drop the Result — low 🟡 — crates/ironrdp-graphics/src/srl.rs
   With MissingTerminator removed, the payload-stripping match in new() has no failing path, so it can return Self. This deletes the Ok wrapper, the propagation in decode_srl, the transpose at progressive.rs line 190, and test unwraps. The PR is already a breaking change (two error variants removed), so the signature change costs nothing extra.
5. [code-compressor] read_bit, read_bits, and decode_zero_run are now infallible; unwrap the Results — low 🟡 — crates/ironrdp-graphics/src/srl.rs
   read_bit returns Ok(false) past the end with no error path, read_bits only forwards it, and decode_zero_run's only propagated errors came from those two, so all three can never fail. Their Result wrappers and propagation at call sites in decode and decode_nonzero are dead plumbing. The tail computation also simplifies to a plain cast: k = kp/8 <= 10, so the value fits usize without try_from/unwrap_or. All private, so no API impact.
6. [code-compressor] decode_nonzero's i16 conversion error branch is unreachable — low 🟡 — crates/ironrdp-graphics/src/srl.rs
   max_magnitude bounds num_bits to 1..=15 so maximum is at most 32767, and the unary loop yields magnitude no greater than maximum, so magnitude always fits i16 and the try_from with MagnitudeOutOfRange cannot fail. Pre-existing code not added by this diff, so lowest priority, but it is unreachable error handling in a file whose change here removes dead error handling.

[Raj singh chauhan (rajchauhan28)]

Verified on Windows Server 2022 (Standard, build 20348.5622) with the graphics pipeline on and no H.264 decoder. On master (966a842), the session ends at the first progressive upgrade pass with Srl(MissingTerminator). With this PR (f10d8d6) merged, the first screen paints completely. Resizes after that still need the bitmap cache kept across ResetGraphics (#2011 or #1977). Details and numbers: #1977 (comment)

Tested with AI assistance; results are from automated runs against the server.

[chatgpt-codex-connector]

The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins.

[AKolenda]

Pushed f47878a to address the latest review. The decoder no longer strips a final zero byte before reading coefficients, and it preserves a pending nonzero value when its sign or magnitude reaches EOF. Added the reviewed [0x86, 0x00] case, a pending value crossing a band boundary, and round trips with and without terminators. Removed the redundant exhaustion/cap logic and corrected the truncation documentation and PR description.

All 245 graphics library tests, targeted Clippy with warnings denied, and formatting passed. These tests run in normal workspace CI.

I did not add a corruption log: omitted trailing entries and truncation have the same zero-filled representation, so the decoder cannot reliably distinguish them. The documentation now states that limit.

[AKolenda]

The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins.

ignore this, my external review agent, idk why it activated on external PR

[AKolenda]

Rechecked the failed notification against the latest commit: the normal build/test CI suite passes. The separate public API check fails before comparing changes because its fresh dependency resolution selects incompatible picky-krb 0.12.5 with sspi 0.21.3. The focused workflow repair is #2071, which builds both revisions with their committed lockfiles. It has passed a real IronRDP API build and unchanged/breaking/stale-lockfile fixtures locally. The workflow runs from the base branch, so this check needs that repair merged before a rerun can use it.

[Benoît Cortier (CBenoit)]

PR automation is failing because of a picky-krb 0.12.5 incompatibility, fixed on master by #2074. Please rebase on master to fix it.

Update: no rebase needed anymore. picky-krb 0.12.5 was yanked from crates.io (re-released as 0.13.0), so the API check builds again without changes to this branch. PR automation has been re-run here and passes.

[AKolenda]

Rebased on master. On the overlap notice: this PR and #2085 are complementary. Neither duplicates the other's fix.

• This PR changes only SRL parsing: srl.rs and the SrlDecoder construction and tests in decode_upgrade_pass.
• fix(graphics): read the DWT variant from REGION flags #2085 changes only how ProgressiveDecoder::decode_bitmap picks the DWT variant: REGION flags instead of CONTEXT flags, and CONTEXT is no longer required. It does not touch decode_upgrade_pass or srl.rs.

The DWT variant applies after coefficients are reconstructed, so neither change affects the other's behaviour. They merge without conflicts in either order. On the merged result, the ironrdp-graphics, ironrdp-egfx and ironrdp-testsuite-core tests pass.

[github-actions]

PR #2010 relaxes SRL decoding for RFX Progressive upgrade passes to match Windows-encoded streams: the trailing zero byte is no longer stripped or validated, bits past EOF read as zeros, and zero-run events are consumed incrementally without a decoder-side cap. I independently verified the event-per-iteration state machine is semantically equivalent to the previous accumulate loop (same KP updates), the decode loop terminates, the expect() in decode_nonzero is unreachable (num_bits <= 15 bounds magnitude at 32767), and the breaking API change (SrlDecoder::new now infallible; MissingTerminator/Truncated removed) has no remaining in-repo consumers. The three published findings are all low-severity: the by-design loss of truncation detection (documented, matches the reference decoder's zero-filled accumulator), a vestigial Option<SrlDecoder>/has_srl_values gate left from the old fallible constructor, and a leftover u16 return width in BitReader::read_bits forcing a conversion at its only…

1. [code-compressor] Option<SrlDecoder> and has_srl_values gate are vestigial now that SrlDecoder::new is infallible — low 🟡 — crates/ironrdp-graphics/src/progressive.rs
   The PR made SrlDecoder::new infallible but kept the Option wrapper in decode_upgrade_pass: has_srl_values (an any-closure over bands) gates has_srl_values.then(|| SrlDecoder::new(srl_data)), and the loop matches on as_mut() with a None => Vec::new() fallback. The gate is redundant because when it is false every non-LL3 band with num_bits != 0 has zero_count == 0, and decode(0, num_bits) returns an empty Vec without consuming bits or validating num_bits. An unconditional SrlDecoder::new plus decode is behavior-preserving and deletes the closure, .then(...), and match (~11 lines, one branch); a short comment can carry the decode-only-when-values-exist intent. Null lines: only line 191 of this region is added by the PR.
2. [code-compressor] read_bits returning u16 forces a usize::from conversion at its only call site — low 🟡 — crates/ironrdp-graphics/src/srl.rs
   read_bits was narrowed to u16 in this PR after the removed overflow-cap checking, but its sole decode call site (decode, line 97) immediately converts again with usize::from(self.reader.read_bits(k)). Since k = kp/8 is at most 10 (MAX_KP = 80), the value always fits; having read_bits return usize directly removes the conversion and matches the natural width for a run length, with no behavior change. Null lines: line 97 is added by the diff while the read_bits signature and body are mostly unchanged context.

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

[github-actions]

[skeptical] Removed truncation detection makes corrupt streams decode as maximum-magnitude coefficients — low 🟡 — read_bit now returns zeros past EOF with no error path, so a truncated or corrupt stream is silently accepted: a pending value whose sign/magnitude bits fall past the end decodes as the positive maximum (the PR's own tests assert coefficients[0] == 15 and SIGN_POSITIVE), and cut-off run tail bits read as zeros. Impact is limited to rendered pixel data: coefficients are clamped i16, num_values is bounded by band counts, and there is no memory-safety or non-termination exposure. The limitation is documented in SrlDecoder::new and decode_upgrade_pass and is inherent to the zero-filled-reader design that fixes the Windows interop failure; recorded so the loss of the malformed-stream signal is a conscious acceptance.