fix(pdu): keep the packed byte of RLEX segments when the palette has a single entry - #1903
Conversation
…a single entry MS-RDPEGFX 2.2.4.6.2.2 gives stopIndex floor(log2(paletteCount - 1)) + 1 bits; with a single palette entry FreeRDP's CLEAR_LOG2_FLOOR table yields 0, so stopIndex still takes one bit and every segment keeps its packed byte. The single-entry shortcut read bare run lengths instead, one byte off, and failed with "suite exceeds region pixel count" on the solid taskbar corners Windows Server encodes this way (14x64, 64x46). Closes Devolutions#1902
GitHub Actions
Inactive
github-actions
Bot
added
kind/protocol
There was a problem hiding this comment.
🟡 Changes recommended
The full test suite fails because an existing one-entry RLEX fixture was not updated.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes single-entry RLEX palette decoding by preserving the mandatory packed segment byte.
Changes:
- Uses one
stopIndexbit for single-entry palettes. - Removes the obsolete bare-run decoder path.
- Adds a Windows Server regression test.
File summaries
| File | Review |
|---|---|
crates/ironrdp-pdu/src/codecs/clearcodec/rlex.rs |
Corrects RLEX parsing. The existing subcodec fixture must include the packed byte and updated bitmap length. Renaming the helper to decode_segments is a non-blocking nit. |
Review details
Suppressed comments (1)
crates/ironrdp-pdu/src/codecs/clearcodec/rlex.rs:87
- Now that this path also handles one-entry palettes,
decode_multi_palette_segmentsno longer describes the helper's responsibility. Rename it todecode_segmentsso future changes do not mistakenly assume the helper is only valid for multi-entry palettes.
decode_multi_palette_segments(
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| &mut segments, | ||
| )?; | ||
| } | ||
| decode_multi_palette_segments( |
There was a problem hiding this comment.
Thanks for looking. On this branch cargo test -p ironrdp-pdu passes (423 tests, and the CI checks are green): decode_subcodec_layer only records each region with its codec id and never runs the RLEX decoder on it, so this fixture was not parsed. It did still describe the old encoding, so 399ea8c makes it a valid one-entry RLEX stream (bitmap length 6, a zero packed byte before the run length), as you suggested.
The subcodec layer only records the region and its codec id, so this fixture is never parsed by the RLEX decoder and the test kept passing. It still documented the old, incorrect encoding (no packed byte when the palette has a single entry); make it match what the decoder now expects so the fixture stays a valid RLEX stream.
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Automated review will not run because this contributor is not yet eligible under the automation policy. Contributors become eligible after one qualifying IronRDP pull request is merged into |
Closes #1902.
decode_rlexspecial-cased a one-entry palette (stop_index_bits = 0, bare run lengths). Per MS-RDPEGFX 2.2.4.6.2.2 and FreeRDP'sclear_decompress_subcode_rlex(CLEAR_LOG2_FLOOR[0] == 0),stopIndexstill takes one bit in that case and every segment keeps its packed byte. The shortcut read every run length one byte off and rejected the one-colour regions Windows Server uses for the solid corners of its taskbar (14×64, 64×46) with "suite exceeds region pixel count".This PR gives
stopIndexone bit for a single entry, removes the special path, and adds a test decoding such a 14×64 region (a run of 895 on a 16-bit length plus a suite of one). No encoder emitted the old form.cargo test -p ironrdp-pdu rlex: 8 passed.Shipped in avash's vendored
ironrdp-pdusince 0.7.1, verified against Windows Server 2025 with the recorded session that exposed it.