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.