fix(session)!: always own a bulk decompressor for FastPath updates

[Greg Lamberson (glamberson)]

Fixes the session crash when a server sends a compressed FastPath update without compression having been negotiated (for example a full-frame redraw after a resize). Closes #1193.

Previously the FastPath Processor held an Option<BulkCompressor> that was None unless compression was negotiated, and a compressed update with no decompressor was forwarded as raw compressed bytes, which the PDU decoder then misread as enormous length fields and aborted the session.

This makes the bulk decompressor mandatory and library-owned:

• ProcessorBuilder no longer takes a bulk_decompressor; Processor always constructs one in build(). There is no None and no drop path: compressed updates are always decodable.
• The construction-time compression type is irrelevant on the receive path (decompress routes per update on the packet's type bits), so the decompressor is built with a fixed default.
• BulkCompressor::new is now infallible. Its only failure path was an internal self-check on NCRUSH's static Huffman tables (a compile-time invariant), now expressed as debug_assert!. This removes the silent-fallback-on-error path entirely.
• ironrdp-client (including the Deactivation-Reactivation rebuild, which is the resize path), ironrdp-web, ffi, and the test suites are updated to the new builder shape. The reactivation path previously passed None, so this also closes a silent-drop-after-resize gap.

Adds a regression test (ironrdp-testsuite-core/tests/session/fast_path.rs) that renders the same bitmap update plain and bulk-compressed through fresh processors and asserts identical framebuffers.

BREAKING CHANGE: fast_path::ProcessorBuilder no longer has a bulk_decompressor field; the decompressor is always created internally. ironrdp_bulk::BulkCompressor::new now returns Self instead of Result<Self, BulkError>.

[Greg Lamberson (glamberson)]

Benoît Cortier (@CBenoit) Ping. Could we get this merged? Any concerns?

[Copilot]

Pull request overview

This PR fixes session crashes caused by Microsoft RDP servers sending compressed FastPath updates even when the client did not negotiate compression, by ensuring the FastPath pipeline can safely handle (or reject) compressed updates.

Changes:

• Always initialize a BulkCompressor for FastPath processing in ActiveStage::new, defaulting to Rdp61 when no compression type was negotiated.
• Change FastPath handling so that receiving compressed updates without a configured decompressor drops the update (with a warning) instead of forwarding compressed bytes downstream and corrupting decoding.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
crates/ironrdp-session/src/fast_path.rs	Drops compressed FastPath updates when no decompressor is configured to prevent downstream decode corruption/crashes.
crates/ironrdp-session/src/active_stage.rs	Initializes a bulk decompressor unconditionally so compressed FastPath updates can be decompressed even when compression wasn’t negotiated.

---

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

[Benoît Cortier (CBenoit)]

Thanks for the detailed write-up, the diagnosis is accurate, it was helpful. My understanding is that the fix is directionally right, but I think there are a few things worth addressing before merging:

1/ The Rdp61 fallback assumption.

The fix hinges on the claim that a BulkCompressor initialized with Rdp61 correctly dispatches to whatever compression type the per-update FastPath header specifies. BulkCompressor is stateful (sliding window), so the question is: does an Rdp61 context initialized cold, with no negotiation phase, produce correct output if the server sends RDP40- or RDP50-tagged updates? If not, we've changed the failure mode from session crash to silent garbage. I would like confirmation this is a non-issue.

2/ No regression tests

The new drop-without-crash behavior on the None decompressor path is deterministic and straightforward to cover: construct a Processor with None, feed it a mock compressed FastPath PDU, assert Ok(empty) and that the session survives. The always-init path in ActiveStage::new is similarly testable with a mock ConnectionResult where compression_type is None. Integration-against-real-servers is not a regression safety net.

3/ Silent degradation if BulkCompressor::new fails

If BulkCompressor::new(Rdp61) returns Err, the code logs an error! but ActiveStage::new still returns Ok. The session starts, looks healthy, and silently drops every compressed FastPath update for its lifetime. For a constructor failure that materially degrades session quality, this probably deserves either propagating the error from ActiveStage::new, or at least a documented invariant that BulkCompressor::new(Rdp61) cannot fail in practice (with justification for why). I’m willing to be convinced otherwise though!

4/ On the negotiated = false log level and the Option<BulkCompressor> API shape

The PR notes preserving Option<BulkCompressor> as a goal ("no API change"), but I think it's worth stepping back and asking what the intended contract is before locking that in.

If the intent is that Rdp61 is a sane default and consumers should never need to think about this, i.e. "always create a decompressor, the library handles it", then the Option is misleading. It implies that None is a legitimate operational choice, when in practice passing None now silently drops compressed updates.

The cleaner resolution would be to remove the Option entirely and require a BulkCompressor from the consumer (a breaking change, but I think it’s honest).

Right now I think it's in an uncomfortable middle ground: the API accepts None, the init path silently falls back to None on error, and the only signal in both cases is a log line at a level that won't be seen. Could you clarify the intended contract? That would help decide whether the Option should go away, or whether the fallback/drop behavior just needs better documentation and louder logging.

[Greg Lamberson (glamberson)]

Thanks, this was a useful review. I ended up reshaping the PR around your 4/ question, since the contract turned out to be the crux. Point by point:

1/ Rdp61 fallback: confirmed non-issue. The construction-time type never participates in decompression. BulkCompressor::new allocates all three receive contexts unconditionally; compression_level is read only by compress() (outbound). decompress() routes per call on the packet's type nibble via CompressionType::from_flags, so each algorithm keeps its own persistent history and they are never mixed. "Cold, no negotiation" is the normal decompression state (no priming handshake; PACKET_AT_FRONT/PACKET_FLUSHED reset history at boundaries). An unknown type nibble returns Err, surfacing as a session error, not silent garbage. This is moot anyway after 4/, since the decompressor is no longer built from a negotiated type.

4/ and 3/ The contract: the library now owns the decompressor. You were right that Option/None was an uncomfortable middle ground, so I took the honest path you floated. ProcessorBuilder no longer takes a bulk_decompressor; Processor always constructs one in build(). No None, no drop branch, no ambiguity. Contract: a FastPath Processor always owns a bulk decompressor; compressed updates are always decodable. This also closes a real gap your question surfaced: the Option was public and ironrdp-client rebuilt the processor with None on the Deactivation-Reactivation Sequence (the resize path, i.e. the crash scenario), so the original fix prevented the crash but the client would then silently drop the compressed redraws. With build() owning the decompressor, that is fixed by construction.

On 3/: with construction owned by the library, the only fallible step was BulkCompressor::new, whose single failure path is an internal self-check on NCRUSH's static Huffman tables (a compile-time invariant, not a runtime condition). Per your point on #1267/#1274, I made BulkCompressor::new infallible and converted those self-checks to debug_assert!, so there is no error to swallow and no expect() in the path.

2/ Regression test. Added ironrdp-testsuite-core/tests/session/fast_path.rs: it renders the same bitmap FastPath update twice, plain and bulk-compressed, through fresh processors and asserts byte-identical framebuffers (plus a check that pixels actually changed). It covers the reactivation path too, since that also goes through build().

Breaking-change footprint: ProcessorBuilder loses its public bulk_decompressor field, and ironrdp_bulk::BulkCompressor::new becomes infallible (Result<Self, _> to Self).

One side-finding, not in this PR: while writing the test I hit a latent bug in FastPathUpdatePdu::encode, which writes the update header byte before setting the COMPRESSION_USED bit, so a compressed fast-path PDU encodes without the header flag and won't round-trip. Nothing in-tree encodes compressed fast-path output, so it is never exercised. I will send it as its own small fix(pdu).