Skip to content

feat(pdu,graphics,egfx): add ClearCodec bitmap compression codec#1174

Open
Greg Lamberson (glamberson) wants to merge 1 commit into
Devolutions:masterfrom
lamco-admin:feat/clearcodec-codec
Open

feat(pdu,graphics,egfx): add ClearCodec bitmap compression codec#1174
Greg Lamberson (glamberson) wants to merge 1 commit into
Devolutions:masterfrom
lamco-admin:feat/clearcodec-codec

Conversation

@glamberson
Copy link
Copy Markdown
Contributor

Part of the multi-codec EGFX implementation described in #1158 (Section 3).

Summary

Add ClearCodec (MS-RDPEGFX 2.2.4.1), the mandatory lossless bitmap codec
for all EGFX versions (V8-V10.7). ClearCodec uses three-layer compositing
(residual BGR RLE, bands with V-bar caching, subcodec regions) to encode
text, UI elements, and icons with pixel-perfect fidelity.

This is the first ClearCodec encoder in any open-source RDP implementation.
FreeRDP has client-side decode only.

What it adds

ironrdp-pdu (wire format):

  • codecs/clearcodec/: ClearCodecBitmapStream, CompositePayload, residual
    RLE decode/encode, bands with V-bar cache references (full hit, short
    hit, short miss), RLEX subcodec, raw/NSCodec dispatch
  • 24 inline unit tests for wire format parsing

ironrdp-graphics (codec logic):

  • clearcodec/: ClearCodecDecoder with persistent V-bar and glyph caches,
    ClearCodecEncoder with residual-only encoding and glyph deduplication
  • VBarCache: 32,768 full + 16,384 short V-bar ring buffers per spec
  • GlyphCache: 4,000 entries, area <= 1024 pixels per spec
  • 18 inline unit tests for cache operations

ironrdp-egfx (server integration):

  • GraphicsPipelineServer::send_clearcodec_frame()

ironrdp-testsuite-core:

  • 35 integration tests: codec round-trip with pixel-perfect verification,
    adversarial input handling (max run_length, OOM dimensions, malformed
    streams, uncached glyph hits), bands layer compositing through the full
    decode pipeline, cache state management across multi-frame sessions,
    compression quality assertions

Security hardening

Three rounds of code review against FreeRDP ClearCodec CVEs:

  • Cap residual run_length to output buffer (GHSA-32q9-m5qr-9j2v)
  • Cap allocation to 8192x8192 pixels (CVE-2026-26955 class)
  • Fix shortVBarYOff: absolute end-row offset, not pixel count delta
    (MS-RDPEGFX 2.2.4.1.1.2.1.1.3)
  • Validate glyph index range 0-3999 (CVE-2026-23531 class)
  • Validate shortVBarYOff >= shortVBarYOn

NSCodec

The NSCodec subcodec (Layer 3 option) is not implemented in this PR.
The encoder avoids generating NSCodec tiles, and the decoder stub leaves
NSCodec regions at their residual-layer values. This can be added later
without API changes.

Test plan

  • cargo xtask check fmt/lints/tests/typos/locks (all pass)
  • 77 total tests (35 testsuite + 42 inline)

Greg Lamberson (glamberson) added a commit to lamco-admin/IronRDP that referenced this pull request Mar 18, 2026
Wire ClearCodec into the EGFX client's WireToSurface1 codec dispatch,
following the same pattern as AVC420 and Uncompressed decode.

- Add ClearCodecDecoder field (always enabled, no external codec needed)
- Decode ClearCodec bitmap data and convert BGRA output to RGBA
- Reset decoder caches on ResetGraphics (V-bar + glyph state)
- Add 3 integration tests: basic decode, RGBA output, reset survival
- Add unit test for BGRA-to-RGBA channel reordering

Depends on the ClearCodec codec PR (Devolutions#1174).
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds ClearCodec (MS-RDPEGFX 2.2.4.1) support across the wire-format layer (ironrdp-pdu), codec logic (ironrdp-graphics), and EGFX server API (ironrdp-egfx), with extensive new tests in ironrdp-testsuite-core.

Changes:

  • Introduces ironrdp-pdu::codecs::clearcodec with residual/bands/subcodec parsing and helpers.
  • Adds ironrdp-graphics::clearcodec encoder/decoder plus persistent V-bar and glyph caching.
  • Extends GraphicsPipelineServer with send_clearcodec_frame() and adds ClearCodec integration tests.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
crates/ironrdp-testsuite-core/tests/graphics/mod.rs Registers the new ClearCodec integration test module.
crates/ironrdp-testsuite-core/tests/graphics/clearcodec.rs Adds ClearCodec end-to-end and adversarial integration tests.
crates/ironrdp-pdu/src/codecs/mod.rs Exposes the new ClearCodec codec module.
crates/ironrdp-pdu/src/codecs/clearcodec/mod.rs Defines ClearCodec bitmap stream + composite payload parsing and exports layer decoders.
crates/ironrdp-pdu/src/codecs/clearcodec/residual.rs Implements residual (BGR RLE) encode/decode.
crates/ironrdp-pdu/src/codecs/clearcodec/bands.rs Implements bands layer decoding (V-bar references + inline data).
crates/ironrdp-pdu/src/codecs/clearcodec/subcodec.rs Implements subcodec layer parsing (raw/NSCodec/RLEX region headers).
crates/ironrdp-pdu/src/codecs/clearcodec/rlex.rs Implements RLEX subcodec parsing.
crates/ironrdp-graphics/src/lib.rs Exposes the new clearcodec module.
crates/ironrdp-graphics/src/clearcodec/mod.rs Implements ClearCodec decoder/encoder and compositing across layers.
crates/ironrdp-graphics/src/clearcodec/glyph_cache.rs Adds glyph cache backing storage used by encoder/decoder.
crates/ironrdp-graphics/src/clearcodec/vbar_cache.rs Adds V-bar/short V-bar cache backing storage used by decoder.
crates/ironrdp-egfx/src/server.rs Adds send_clearcodec_frame() EGFX server API entry point.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread crates/ironrdp-pdu/src/codecs/clearcodec/mod.rs
Comment thread crates/ironrdp-graphics/src/clearcodec/vbar_cache.rs Outdated
Comment thread crates/ironrdp-egfx/src/server.rs
Comment thread crates/ironrdp-graphics/src/clearcodec/mod.rs Outdated
Comment thread crates/ironrdp-graphics/src/clearcodec/mod.rs
Comment thread crates/ironrdp-graphics/src/clearcodec/mod.rs Outdated
Comment thread crates/ironrdp-graphics/src/clearcodec/mod.rs
Comment thread crates/ironrdp-graphics/src/clearcodec/mod.rs Outdated
Greg Lamberson (glamberson) added a commit to lamco-admin/IronRDP that referenced this pull request Mar 18, 2026
…items

Address review feedback on Devolutions#1174:

- Validate subcodec region bounds against surface dimensions
- Validate RLEX palette indices and enforce region pixel budget
- Remove silent pixel skipping that caused coordinate desync
- Add glyph cache dimension mismatch check on hit
- Use checked arithmetic for raw subcodec data length
- Remove unused expected_seq field from ClearCodecDecoder
- Use saturating_mul in encoder for pixel count
- Add missing QoE backpressure recording for ClearCodec frames
- Document vbar_cache wrap constant derivation
Greg Lamberson (glamberson) added a commit to lamco-admin/IronRDP that referenced this pull request Mar 28, 2026
…items

Address review feedback on Devolutions#1174:

- Validate subcodec region bounds against surface dimensions
- Validate RLEX palette indices and enforce region pixel budget
- Remove silent pixel skipping that caused coordinate desync
- Add glyph cache dimension mismatch check on hit
- Use checked arithmetic for raw subcodec data length
- Remove unused expected_seq field from ClearCodecDecoder
- Use saturating_mul in encoder for pixel count
- Add missing QoE backpressure recording for ClearCodec frames
- Document vbar_cache wrap constant derivation
Greg Lamberson (glamberson) added a commit to lamco-admin/IronRDP that referenced this pull request Mar 28, 2026
Wire ClearCodec into the EGFX client's WireToSurface1 codec dispatch,
following the same pattern as AVC420 and Uncompressed decode.

- Add ClearCodecDecoder field (always enabled, no external codec needed)
- Decode ClearCodec bitmap data and convert BGRA output to RGBA
- Reset decoder caches on ResetGraphics (V-bar + glyph state)
- Add 3 integration tests: basic decode, RGBA output, reset survival
- Add unit test for BGRA-to-RGBA channel reordering

Depends on the ClearCodec codec PR (Devolutions#1174).
Greg Lamberson (glamberson) added a commit to lamco-admin/IronRDP that referenced this pull request Apr 1, 2026
…items

Address review feedback on Devolutions#1174:

- Validate subcodec region bounds against surface dimensions
- Validate RLEX palette indices and enforce region pixel budget
- Remove silent pixel skipping that caused coordinate desync
- Add glyph cache dimension mismatch check on hit
- Use checked arithmetic for raw subcodec data length
- Remove unused expected_seq field from ClearCodecDecoder
- Use saturating_mul in encoder for pixel count
- Add missing QoE backpressure recording for ClearCodec frames
- Document vbar_cache wrap constant derivation
Greg Lamberson (glamberson) added a commit to lamco-admin/IronRDP that referenced this pull request Apr 1, 2026
Wire ClearCodec into the EGFX client's WireToSurface1 codec dispatch,
following the same pattern as AVC420 and Uncompressed decode.

- Add ClearCodecDecoder field (always enabled, no external codec needed)
- Decode ClearCodec bitmap data and convert BGRA output to RGBA
- Reset decoder caches on ResetGraphics (V-bar + glyph state)
- Add 3 integration tests: basic decode, RGBA output, reset survival
- Add unit test for BGRA-to-RGBA channel reordering

Depends on the ClearCodec codec PR (Devolutions#1174).
@glamberson Greg Lamberson (glamberson) force-pushed the feat/clearcodec-codec branch 2 times, most recently from 973f325 to 662548b Compare April 1, 2026 15:03
@GlassOnTin
Copy link
Copy Markdown
Contributor

Reviewing against Haven's downstream ClearCodec decoder — your factorisation (separate glyph_cache.rs / vbar_cache.rs / per-layer PDU modules) is cleaner than what we shipped (single-file ~600 LOC), and the test coverage is broader. No bugs spotted in the decoder pass.

Note that the RDPGFX_RECT16 exclusive-vs-inclusive issue I raised on #1175 affects the caller (the EGFX client dispatch) rather than this PR — the decoder itself takes raw width: u16, height: u16 so it's unaffected. Flagging here just to cross-link.

Haven would drop our rdp-kotlin/rust/src/egfx/clear.rs once these merge and a new ironrdp-graphics reaches crates.io.

@glamberson
Copy link
Copy Markdown
Contributor Author

Thanks for the careful review against Haven's downstream implementation, that is exactly the kind of independent confirmation that helps these PRs land. The factoring into separate glyph_cache.rs / vbar_cache.rs / per-layer PDU modules came out of writing the round-trip test fixtures, since each cache layer ended up wanting its own targeted tests.

Cross-linking your RDPGFX_RECT16 finding back to #1175 where the actual fix lives, and confirming this PR's decoder is unaffected since it takes raw width: u16, height: u16 rather than reading from a rect.

@glamberson
Copy link
Copy Markdown
Contributor Author

Force-pushed 43d1f14e -> 6e142c37 with three pre-review hardening fixes. All prior review threads remain resolved (additive changes only); the original 9 Copilot threads + GlassOnTin's Haven review are unaffected.

Bugs fixed

1. Integer overflow panic in bands::decode_single_band at crates/ironrdp-pdu/src/codecs/clearcodec/bands.rs:107. usize::from(x_end - x_start + 1) was u16 arithmetic; with x_end = u16::MAX and x_start = 0, the + 1 overflowed before the usize cast and triggered a release-mode panic under workspace overflow-checks. The existing if x_end < x_start guard prevents subtraction underflow but not the + 1 overflow. Fix: cast first, usize::from(x_end - x_start) + 1.

2. Adversarial-dimensions OOM in ClearCodecDecoder::decode. The previous per-pixel-count cap (MAX_DECODE_PIXELS = 8192 * 8192 = 67M pixels) accepted degenerate aspect ratios like 63961 x 771 (49M pixels) that allocated ~197MB from a handful of attacker-controlled bytes. Replaced with a per-DIMENSION cap (each axis <= 8192). Still supports 8K displays at 7680x4320 with headroom; MS-RDPEGFX caps surfaces at 32767x32767 but does not mandate a separate tile cap.

3. Alpha-channel contract was undocumented on ClearCodecEncoder::encode and ClearCodecDecoder::decode. ClearCodec is lossless on BGR per spec; the wire format does not transmit alpha. The encoder reads B/G/R from each input pixel and discards alpha; the decoder fills output alpha with 0xFF unconditionally. Previously "pixel-perfect fidelity" in the body was ambiguous about which channels. Documented explicitly on both public methods.

Spec audit (MS-RDPEGFX 2.2.4.1)

While preparing the fixes I also re-read MS-RDPEGFX 2.2.4.1 against the implementation, using FreeRDP 3.15.0's clear_decompress as a corroborating data point (one other implementation, not the reference; the spec is). Conclusion: IronRDP's clearcodec layer-decoders are spec-conformant in the bytes-remaining checks. The spec defines three explicit 4-byte length-prefix fields at the COMPOSITE_PAYLOAD outer layer (residualByteCount, bandsByteCount, subcodecByteCount per 2.2.4.1.1); CompositePayload::decode parses all three, sums with overflow check, validates total via ensure_size!, slices exactly those byte counts. Per-layer decoders then receive slices bounded to exactly the announced sizes and use ensure_size! per-read within the bounded slice. Subcodec also uses the spec's per-region bitmapDataByteCount field.

IronRDP applies these length checks incrementally per-read; another implementation pattern is to apply them upfront in a single shot per layer entry-point. Both styles satisfy the spec-required bounded-read outcome; the spec does not require either over the other.

How these were surfaced

For transparency: the three bugs were surfaced by local fuzz oracles (decode + encode/decode round-trip on the codec's public API) before maintainer review. The fuzz infrastructure is on a separate branch and will follow as a small standalone PR after this one merges; the codec's public API is already fuzz-friendly so no prep work is needed in this PR.

Verification post-amend: cargo xtask check fmt/lints/tests/typos/locks all pass; the 35 testsuite-core ClearCodec tests pass under the new dimension cap and overflow fix.

@CBenoit
Copy link
Copy Markdown
Member

Benoît Cortier (CBenoit) commented May 25, 2026

Greg Lamberson (@glamberson) Thank you very much. Just confirming with you: can I go ahead and merge this PR, or any prerequisite I’m missing?

Add ClearCodec (MS-RDPEGFX 2.2.4.1) codec support across two crates:

ironrdp-pdu: Wire-format decode/encode for all ClearCodec layers
(residual BGR RLE, bands with V-bar caching, subcodec dispatch
including RLEX palette-indexed RLE). Full round-trip test coverage.

ironrdp-graphics: ClearCodecDecoder with persistent V-bar and glyph
caches. ClearCodecEncoder with residual-only encoding and glyph
deduplication for server-side bitmap compression.

Hardening pass before maintainer review (local fuzz oracles surfaced
three bug classes that escaped the initial review):

- Per-dimension cap at 8192 on decode, replacing the previous per-
  pixel-count cap at 8192*8192. The pixel-count form accepted
  degenerate aspect ratios (e.g. 63961x771 = 49M pixels, under the
  67M cap) that allocated ~197MB from a few attacker-controlled
  bytes. The per-dimension form rejects implausible tile shapes
  regardless of total area while preserving headroom for 8K
  displays (7680x4320). MS-RDPEGFX caps surfaces at 32767x32767
  but does not mandate a separate tile cap; 8192 is a defensive
  choice that fits realistic tile sizes.

- Fix integer overflow at bands::decode_band on
  `usize::from(x_end - x_start + 1)`. When x_end = u16::MAX and
  x_start = 0, the `+ 1` overflowed the u16 arithmetic before the
  usize cast and triggered a release-mode panic under
  overflow-checks. The existing `if x_end < x_start` guard
  prevents underflow but not the +1 overflow. Fix is to cast to
  usize first: `usize::from(x_end - x_start) + 1`.

- Document the alpha contract on ClearCodecEncoder::encode and
  ClearCodecDecoder::decode. ClearCodec is lossless on the three
  color channels (B, G, R) per spec; the wire format does not
  transmit alpha. The encoder reads B/G/R from each input pixel
  and discards alpha; the decoder fills alpha with 0xFF
  unconditionally. Callers needing alpha must transport it
  separately. Previously the doc-comment claim of "pixel-perfect
  fidelity" was ambiguous; alpha-channel mismatches in any
  round-trip with non-0xFF alpha inputs are by design, not bugs.

Local fuzz infrastructure (clearcodec_decode + clearcodec_round_trip
oracles) follows in a separate PR once this lands; the codec's
public API is already fuzz-friendly so no prep is needed here.

A separate spec-driven audit of input-validation completeness
(per-layer stream-length pre-checks in subcodec / residual /
bands) is queued as follow-up work; deferring from this PR pending
review of MS-RDPEGFX 2.2.4.1.

Closes Devolutions#1158 ClearCodec subtask.
@glamberson
Copy link
Copy Markdown
Contributor Author

Benoît Cortier (@CBenoit) Yes, no prerequisite blocks merge. Rebasing onto current master (879ffed8) now to absorb today's #1305 (RawCapabilitySet split) and #1303 (egfx duplicates removal); will push once CI re-runs green. The follow-up clearcodec fuzz oracle stack (clearcodec_decode + clearcodec_round_trip) lives on feat/clearcodec-fuzz and lands as a separate PR, not a prerequisite for this one. F4a-c FreeRDP-differential candidates are intentionally deferred pending an MS-RDPEGFX §2.2.4.1 audit per the 2026-05-20 SDS addendum.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants