Repository navigation
feat(fuzz): add egfx_surface_state state-machine oracle - #1337
Conversation
4939a82 to
e6e38d2
Compare
e6e38d2 to
baef3ba
Compare
a136b61 to
7ad4d48
Compare
7ad4d48 to
e297daa
Compare
e297daa to
2b8168e
Compare
2b8168e to
5dbb72c
Compare
|
Non-blocking medium finding: the EGFX surface-state oracle removes deleted or reset surface IDs from its expected model but only validates IDs still expected, so a client that retains such a surface can pass despite the oracle documentation stating it detects those regressions. |
5dbb72c to
ab13980
Compare
There was a problem hiding this comment.
Test-infrastructure-only PR adding an egfx surface-lifecycle fuzz oracle with a parallel state model. Two blocking issues. First, the assertion loop only checks model-subset-of-client, so the doc comment's headline guarantee (client retains a surface after DeleteSurface) and the ResetGraphics clear-all bug class are undetectable. Second, the oracle applies the model transition on encode success, but `ResetGraphicsPdu::encode` omits the bounds its decoder enforces, so nearly every generated ResetGraphics clears the model while the client keeps its surfaces, permanently degrading the oracle and falsifying the stated no-desync invariant. Also flagged: the pre-existing `padding_size` underflow that now panics inside the harness far more often, a model that only restates implementation policy the spec leaves open, an unmodeled variant inflating the density claim, and an empty seed that gives the CI regression test no coverage.
Protocol analysis: partially_accepted — Confirmed both material items in pr-head. The missing deleted-surface assertion is real: the loop iterates only surviving modeled surfaces and `get_surface` is the sole accessor. The encode-implies-dispatch item is real: `ResetGraphicsPdu::encode` writes width, height and nMonitors unchecked while `decode` enforces 32766 and 16; the `padding_size` underflow is reachable via `size()` from `encode_vec`. Accepted the clear-all-on-reset and zero-dimension CreateSurface points, but recorded them as non-blocking since both are pre-existing in ironrdp-egfx. Refined the empty-seed item (real gap, but the established convention for new targets) and the FrameAcknowledge item (doc accuracy, not a protocol conflict). The other six variants round-trip, so ResetGraphics is the only desync source.
- non_blocking / low — crates/ironrdp-testsuite-core/test_data/fuzz_regression/egfx_surface_state/seed-empty.bin
The sole corpus entry is zero bytes. `Vec::<SurfaceLifecyclePdu>::arbitrary` on empty input yields an empty vector, so `check_egfx_surface_state` constructs a client and asserts nothing about any modeled transition. An empty seed matches the existing convention for other targets, so this is not a blocker on its own, but for a target whose entire value is the state model, one seed encoding a real CreateSurface, MapSurfaceToOutput, StartFrame, EndFrame, DeleteSurface sequence would make the CI regression test actually guard the transitions the doc comment describes.
afa82b7 to
f41a5b9
Compare
|
Same finding as the review thread below on the post-dispatch assertion loop, already fixed there: added known_surface_ids to track every id ever created, and the loop now iterates that set so a removed id absent from the client is actually checked rather than never queried. |
|
Replaced the empty seed with a real 64-byte one that decodes to a sequence of all five relevant PDU types: EndFrame, StartFrame, CreateSurface, MapSurfaceToOutput, DeleteSurface. Found by searching random buffers through arbitrary's actual decoding, not hand-encoded, then verified: runs clean, no panic. Worth noting the honest limit: the surface IDs across those PDUs don't correlate (each targets a different id), so this seed doesn't specifically exercise create-then-delete-on-the-same-surface, that stays covered by the live fuzz corpus rather than this one deterministic seed. It does now give check_egfx_surface_state real exercise of every variant's encode/decode/model path instead of zero. |
0650f6e to
f26fd5f
Compare
d5d2d91 to
084dfb5
Compare
…der against attacker-controlled input (#1333) ## Summary - Implements target 3 of #1316 (egfx fuzz-coverage umbrella): `egfx_zgfx_decompress` oracle and target. - Target shape mirrors PR #1285's `bulk_*` pattern: panic plus sanitizer oracle on `Decompressor::decompress`, fresh decompressor per iteration so history state does not leak between fuzz inputs. - Ships alongside a complete input-validation audit of the ZGFX decoder, following PR #1271's precedent for "target plus the bugs it surfaces in one PR." ## Hardening Eight input-validation gaps closed in `ironrdp-graphics` (all class-(c) per #1314: reachable via attacker-controlled wire inputs). Each one was surfaced by a rigorous smoke-fuzz iteration: 1. `Bits::try_split_to` (new in `utils.rs`) provides the checked counterpart to `split_to` that every fix below builds on. 2. `SegmentedDataPdu::from_buffer` multipart segment-size: `split_at_checked` plus a defensive `Vec::with_capacity` cap mirroring PR #1271. 3. `decompress_segment` trailing-unused-bits subtraction: `checked_mul` + `checked_sub`. 4. `decompress_segment` token loop: checked prefix indexing + checked `split_to(8)` for the NullLiteral value. 5. `handle_match`: checked distance-bits split, `checked_add` for `distance_base + value`, plus `distance > HISTORY_SIZE` bound check. 6. `read_unencoded_bytes`: checked splits for the 15-bit length, pad-to-boundary, and `length * 8` payload. 7. `read_encoded_bytes`: checked splits + `usize::BITS` bound on `load_be::<usize>()` + `checked_shl` replacing `pow` + `checked_add` for `base + value`. 8. `FixedCircularBuffer::read_with_offset`: defense-in-depth `offset > buffer.len()` bound check below the caller-side guard. Plus memory-budget enforcement, which a match-copy chain makes necessary: it can expand a few bytes of input into unbounded output, and an unbounded run reached a 1.5 GB peak. 9. New `MAX_DECOMPRESSED_PER_SEGMENT = 64 MiB` ceiling on a single segment's decompressed output, enforced per-token in `decompress_segment` and threaded through the match-copy paths so it is checked before any allocation. **This is an implementation resource limit, not a wire requirement**, and the code says so, but MS-RDPEGFX 3.1.9.1.2 ("RDP 8.0 compressor limits") does supply a number: a compliant compressor MUST NOT produce any single segment past 65,535 uncompressed bytes. This crate's production compressor path honors that: `wrapper.rs`'s `wrap_compressed` panics above 65,535, and `compress_and_wrap_egfx`, the only production caller, falls back to uncompressed rather than risk exceeding it. The `compress_high_entropy_round_trips_and_bounds_table` test that an earlier revision of this PR body cited as evidence real traffic exceeds 65,535 does not show that: it calls `Compressor::compress` directly and feeds the result straight to `decompress_segment`, bypassing the wrapper every real sender uses. 64 MiB is still used here, not 65,535, because this ceiling exists to catch non-conforming or hostile input, not to police conforming input: a decoder that hard-rejects at exactly the compressor-side limit has no margin for a peer implementation with a minor, benign spec deviation. Same footing as the compositor's total-byte budget in #1460. 10. Multipart running-total check against the declared `uncompressedSize`, surfacing early detection of segments that collectively exceed the wire-declared bound. Unlike item 9 this one *is* spec-grounded: 2.2.5.1 defines `uncompressedSize` as the size of `segmentArray` once reassembled and decompressed, and 3.1.9.1.2.1 states it MUST equal the total number of decompressed bytes across all segments. The sender side is unchanged and already agreed with the spec: `ZGFX_SEGMENTED_MAXSIZE = 65535` in `wrapper.rs` splits into multipart above that, which is exactly the encoder-side behaviour 3.1.9.1.2.1 describes. Seven new `ZgfxError` variants total with matching Display and `Error::source` impl entries. ## Breaking change `ZgfxError` is a public exhaustive enum, so the seven added variants (`InvalidTrailingBitCount`, `SegmentSizeExceedsBuffer`, `IncompleteBitStream`, `MatchDistanceOutOfRange`, `LengthTokenSizeTooLarge`, `SegmentDecompressedSizeExceedsLimit`, `MultipartTotalExceedsDeclared`) break exhaustive matches downstream. Confirmed by `cargo semver-checks --baseline-rev <merge-base>`: seven `enum_variant_added` failures on `ironrdp-graphics`. Title carries `!` accordingly. ## Validation - `cargo xtask check fmt/lints/tests/typos/locks` all pass. - `check_egfx_zgfx_decompress` regression-replay passes against both shipped crash artifacts. - 46 existing ZGFX unit tests continue to pass against the hardened code. - Final 15-minute rigorous fuzz: 2,942,961 iterations in 901 seconds (~3,266 exec/s sustained), peak RSS 84 MB, zero panics, zero sanitizer reports, zero OOMs. Iterative audit progression on a 15-minute libFuzzer + ASan budget per round: - Round 1 (no fixes): crash at iter ~30 (trailing-bit underflow) - Round 2 (after fixes 1-2): crash at iter ~28 (bit-budget) - Round 3 (after F1-F8 audit): OOM at iter ~9877 (1.5 GB peak) - Round 4 (after F1-F10 complete): clean ## Notes - Crash artifacts for the two header-parse bugs added to `crates/ironrdp-testsuite-core/test_data/fuzz_regression/egfx_zgfx_decompress/`. The later-round crashes are not added as separate regression entries because the libFuzzer corpus accumulated 161 representative inputs and the unit-tested error paths cover the cases directly. - The MS-RDPEGFX 3.1.9.1.2 ("RDP 8.0 compressor limits") per-segment bound is a semantic change: inputs that previously decoded to more than 65,535 bytes per segment now return `Err`. Such inputs were never spec-conformant. Codebase precedent (PR #1097 et seq.) is to add `ZgfxError` variants without a `!` marker on the commit; following that convention. - Memory-budget design discussion lives on #1120 (`issuecomment-4558356535`) rather than in this PR body so future readers find the rationale on the canonical fuzz-umbrella thread. Refs #1316. ## Note on the fuzz numbers above The iteration counts and the 84 MB peak RSS were measured while the per-segment cap was 65,535. Raising it to 64 MiB does not change what the oracle exercises, but those figures describe the earlier revision rather than what ships here. ## Rebase 2026-08-19 Rebased onto current master (was CONFLICTING after the RDPEUDP/RDPEMT series landed). Two conflicts, both purely additive insertions colliding with unrelated new master content at the same point: - `fuzz/Cargo.toml`: master's 8 new RDPEUDP/RDPEMT `[[bin]]` entries plus this PR's own `egfx_zgfx_decompress` entry, both kept. - `crates/ironrdp-fuzzing/src/oracles/mod.rs`: master's 8 new oracle functions plus this PR's own, both appended. The automatic merge dropped this PR's own closing brace for `egfx_zgfx_decompress`, attaching the shared trailing syntax to master's last function instead of both (the same failure mode hit #1337's rebase the other way around, master's brace dropped there). Caught before continuing the rebase this time by diffing the resolved file against both source commits directly; fixed and re-verified byte-identical to both. None of the ZGFX hardening code (`utils.rs`, `zgfx/circular_buffer.rs`, `zgfx/control_messages.rs`, `zgfx/mod.rs`) had any conflict; it applied unchanged. All checks re-run on the new head: `cargo xtask check fmt/lints/tests/typos/locks`, a direct `cargo check --all-targets` in `fuzz/`, and `cargo xtask wasm check`, all green. `fuzz/Cargo.lock` unchanged. ## Review round 2026-08-20 Four findings addressed, three of them posted as review-body text rather than inline threads and never actually worked despite the PR reading as fully resolved. (1) `ZgfxError` marked `#[non_exhaustive]`, matching the same convention already on `DecodeErrorKind`/`EncodeErrorKind` in ironrdp-core; this PR adds several new variants and every downstream exhaustive match would have broken without it. (2) Corrected the doc comment on `MAX_DECOMPRESSED_PER_SEGMENT`: MS-RDPEGFX 3.1.9.1.2's compressor-limits list does supply a bound (65,535 uncompressed bytes per segment for a compliant compressor), and this crate's own compressor honors it (`wrapper.rs` panics above it, the only production caller falls back to uncompressed instead of risking it). (3) Closed a real gap: the multipart aggregate memory budget was only checked against the sender's own declared `uncompressedSize`, a fully attacker-controlled 32-bit field, so a message built from small segments could drive real allocation to roughly 4.29 GiB before that check fired. Added `MAX_DECOMPRESSED_TOTAL` (256 MiB, same footing as `MAX_COMPOSITOR_BYTES` in ironrdp-egfx's compositor.rs), checked independently. (4) Masked the trailer byte to its low 3 bits per MS-RDPEGFX 3.1.9.1.2.4 ("the five high-order bits... are reserved"); reading the whole byte let a sender's nonzero reserved bits get misread as part of the unused-bit count. Both (3) and (4) ship with regression tests negative-verified against the code without the fix. All 6 gates green throughout. ## Rebase 2026-08-20 Rebased onto current master, 9 behind to 0. Two conflicts, both pure unions with master's own new fuzz-target entries in `fuzz/Cargo.toml` and `fuzz_regression.rs`, same shape as the earlier rebase round. `oracles/mod.rs` auto-merged without conflict markers this time; explicitly re-checked it did not repeat the closing-brace-drop failure from that earlier round (the function's own closing brace confirmed present, then a full `cargo check -p ironrdp-fuzzing --all-targets` run clean). All 6 gates green, fuzz workspace lock stable.
084dfb5 to
50ab213
Compare
Implements target 5 of Devolutions#1316 (egfx fuzz-coverage umbrella): a multi-frame oracle that drives a sequence of Arbitrary-derived GfxPdus through a single GraphicsPipelineClient + surface-cache pair across each fuzz iteration, exposing cross-PDU state corruption that single-shot fuzzers cannot reach. Harness shape (per the design note posted as issuecomment-4559459227 on Devolutions#1316). The oracle: 1. Constructs one GraphicsPipelineClient (no-op handler, no H.264 decoder) at iteration start. The surface cache, state machine, and ZGFX decompressor history are owned by this single instance. 2. Initialises the channel by calling DvcProcessor::start so the client moves into its post-start state. 3. Uses arbitrary::Unstructured to derive Vec<GfxPdu> from the raw fuzz input. Each GfxPdu variant is Arbitrary via the cascade in PR Devolutions#1334. 4. For each PDU: encodes back to wire bytes via encode_vec, wraps in a single uncompressed ZGFX segment via wrap_uncompressed, and feeds the wire bytes to the client's public DvcProcessor::process entry point. Errors and panics propagate to libFuzzer naturally. The harness deliberately routes through the public process() entry rather than the private handle_pdu so the dispatch path exercises the same ZGFX-decompress + GfxPdu-decode + per-variant-dispatch + state machine + surface cache flow that production traffic takes. The uncompressed ZGFX wrapper bypasses the ZGFX decoder layer (already covered by egfx_zgfx_decompress in a separate PR) and concentrates fuzz pressure on the dispatch + state machine. What this catches: panics or sanitizer reports along the dispatch + state machine path under adversarially-ordered PDU sequences; inconsistent surface-cache state under attacker-controlled CreateSurface / DeleteSurface / Map* orderings; corrupted frame-id state from interleaved StartFrame / EndFrame / FrameAcknowledge sequences; ZGFX-wrapper integration bugs separate from the standalone ZGFX coverage. What this does NOT catch: cross-frame H.264 decoder state corruption. The client is constructed with h264_decoder: None, so H264-bearing PDUs (WireToSurface1 with AVC codecs) skip the H.264 decoder. The standalone egfx_avc420_decode and egfx_avc444_decode targets cover the H.264 wrapper. Wiring a real (or mock) H.264 decoder into this harness can be a follow-up if frame-to-frame H.264 state coverage surfaces as a gap. Bug discovery during development. The first smoke fuzz on the in-progress harness surfaced a bit_field::set_bits panic in Timestamp::encode where derive(Arbitrary) generated u8/u16 values exceeding the encoder's 6-bit / 10-bit pack widths. The fix landed in PR Devolutions#1334 (the cascade prerequisite) as manual Arbitrary impls for Timestamp, QuantQuality, and the Encoding bitflag, mapping each field to its wire-allowed range. This is the "sanitize via mask" shape from Devolutions#1122's body. Validation. 15-minute rigorous libFuzzer + ASan smoke fuzz on the final SHA: 3,659,157 iterations in 901 seconds (~4,061 exec/s sustained), peak RSS 96 MB, zero panics, zero sanitizer reports, zero OOMs. Corpus grew to ~4,558 entries via libFuzzer's coverage discovery. The regression-replay test check_egfx_multi_frame passes against the seed-empty corpus entry. Depends on PR Devolutions#1334 (Arbitrary cascade across ironrdp-egfx public PDU types). CI on this PR's branch reds until Devolutions#1334 merges or this PR's branch rebases onto master post-merge. That's the correct mechanical signal of the dependency per the no-exclude-then-follow-up rule. Refs Devolutions#1316.
Implements target 6 of Devolutions#1316 (egfx fuzz-coverage umbrella): the egfx_surface_state oracle. Sibling of egfx_multi_frame (target 5, PR 1. The PDU stream is narrowed to a SurfaceLifecyclePdu enum that contains only the seven state-affecting variants: ResetGraphics, CreateSurface, DeleteSurface, MapSurfaceToOutput, StartFrame, EndFrame, FrameAcknowledge. Each iteration concentrates fuzz pressure on the surface and frame state machine rather than spreading across all 23 GfxPdu variants. 2. The oracle maintains a parallel ExpectedState model alongside the client. For each successfully-encoded PDU the model is updated to mirror the client's documented state transition (CreateSurface inserts a surface unless width or height is zero; DeleteSurface removes; MapSurfaceToOutput updates is_mapped and origin; ResetGraphics clears all surfaces; EndFrame increments the running frame counter). After dispatch the client's observable state via the public get_surface and total_frames_decoded getters is asserted against the model. This catches a different bug class from egfx_multi_frame's panic / sanitizer oracle: logic bugs in state transitions that produce wrong but non-crashing observable state. Examples covered: client retains a surface after DeleteSurface, client clobbers is_mapped on a surface that did not receive MapSurfaceToOutput, client fails to clear all surfaces on ResetGraphics, client increments total_frames_decoded on a PDU other than EndFrame. The model encodes the implementation's current documented behavior (e.g., zero-dimension CreateSurface is silently skipped per handle_create_surface), so the oracle catches drift from that behavior. Spec-compliance verification is a separate concern that would require a parallel spec-only model. What this does NOT catch: state transitions that exist only in private fields without a public getter (current_frame_id, frames_queued). Exposing observable access to those fields is a separate egfx-public-API change, out of scope here. Validation. 15-minute rigorous libFuzzer + ASan smoke fuzz on the final SHA: 2,770,751 iterations in 901 seconds (~3,075 exec/s sustained), peak RSS 102 MB, zero panics, zero sanitizer reports, zero state-model assertion failures. Corpus grew to ~1,840 entries via libFuzzer's coverage discovery. The check_egfx_surface_state regression-replay test passes against the seed-empty corpus entry. Stack dependency. This PR is stacked on PR Devolutions#1336 (egfx_multi_frame, target 5) which is stacked on PR Devolutions#1334 (Arbitrary cascade). CI on this branch will red until both ancestors merge or this branch rebases onto master post-merge. With this PR the Devolutions#1316 umbrella has its initial six target slots filled (1 merged, 2+2b+3+5+6 open). Refs Devolutions#1316. Depends on Devolutions#1334 and Devolutions#1336.
Tracks every id CreateSurface has ever inserted in a persistent known_surface_ids set, and matches modeled state against the client's per each: presence still asserts field equality, but absence now asserts the client actually dropped the surface too. Previously the post-dispatch loop only iterated currently-modeled surfaces, so the oracle's advertised retained-after-delete bug class was unreachable. Decodes each PDU's encoded bytes back before mirroring its transition into the model, since encode success does not prove the client will dispatch the PDU: ResetGraphics in particular encodes width and height unchecked while the decoder rejects values above 32766, which Arbitrary-generated u32s almost always exceed. Bounds ResetGraphics's generated monitor list to 16 entries, avoiding a pre-existing padding-size underflow in the encoder that this target's narrower PDU set would otherwise trigger far more often than the sibling target already does. Corrects the module doc's claim that all seven narrowed variants are state-affecting; FrameAcknowledge and StartFrame are not.
The sole corpus entry was zero bytes, so check_egfx_surface_state asserted nothing about any modeled transition. Replaced with a real 64-byte seed found by searching random buffers through arbitrary's actual decoding rather than hand-encoding one: it decodes to a sequence covering all five PDU types the state model tracks (EndFrame, StartFrame, CreateSurface, MapSurfaceToOutput, DeleteSurface), runs clean with no panic. The surface IDs across those PDUs don't correlate, so this does not specifically exercise create-then-delete-on-the-same-surface; that stays covered by the live fuzz corpus. It does give the deterministic regression check real exercise of every variant's encode/decode/model path instead of none.
50ab213 to
3ccad2a
Compare
501d1eb
into
Devolutions:master
…der against attacker-controlled input (Devolutions#1333) - Implements target 3 of Devolutions#1316 (egfx fuzz-coverage umbrella): `egfx_zgfx_decompress` oracle and target. - Target shape mirrors PR Devolutions#1285's `bulk_*` pattern: panic plus sanitizer oracle on `Decompressor::decompress`, fresh decompressor per iteration so history state does not leak between fuzz inputs. - Ships alongside a complete input-validation audit of the ZGFX decoder, following PR Devolutions#1271's precedent for "target plus the bugs it surfaces in one PR." Eight input-validation gaps closed in `ironrdp-graphics` (all class-(c) per Devolutions#1314: reachable via attacker-controlled wire inputs). Each one was surfaced by a rigorous smoke-fuzz iteration: 1. `Bits::try_split_to` (new in `utils.rs`) provides the checked counterpart to `split_to` that every fix below builds on. 2. `SegmentedDataPdu::from_buffer` multipart segment-size: `split_at_checked` plus a defensive `Vec::with_capacity` cap mirroring PR Devolutions#1271. 3. `decompress_segment` trailing-unused-bits subtraction: `checked_mul` + `checked_sub`. 4. `decompress_segment` token loop: checked prefix indexing + checked `split_to(8)` for the NullLiteral value. 5. `handle_match`: checked distance-bits split, `checked_add` for `distance_base + value`, plus `distance > HISTORY_SIZE` bound check. 6. `read_unencoded_bytes`: checked splits for the 15-bit length, pad-to-boundary, and `length * 8` payload. 7. `read_encoded_bytes`: checked splits + `usize::BITS` bound on `load_be::<usize>()` + `checked_shl` replacing `pow` + `checked_add` for `base + value`. 8. `FixedCircularBuffer::read_with_offset`: defense-in-depth `offset > buffer.len()` bound check below the caller-side guard. Plus memory-budget enforcement, which a match-copy chain makes necessary: it can expand a few bytes of input into unbounded output, and an unbounded run reached a 1.5 GB peak. 9. New `MAX_DECOMPRESSED_PER_SEGMENT = 64 MiB` ceiling on a single segment's decompressed output, enforced per-token in `decompress_segment` and threaded through the match-copy paths so it is checked before any allocation. **This is an implementation resource limit, not a wire requirement**, and the code says so, but MS-RDPEGFX 3.1.9.1.2 ("RDP 8.0 compressor limits") does supply a number: a compliant compressor MUST NOT produce any single segment past 65,535 uncompressed bytes. This crate's production compressor path honors that: `wrapper.rs`'s `wrap_compressed` panics above 65,535, and `compress_and_wrap_egfx`, the only production caller, falls back to uncompressed rather than risk exceeding it. The `compress_high_entropy_round_trips_and_bounds_table` test that an earlier revision of this PR body cited as evidence real traffic exceeds 65,535 does not show that: it calls `Compressor::compress` directly and feeds the result straight to `decompress_segment`, bypassing the wrapper every real sender uses. 64 MiB is still used here, not 65,535, because this ceiling exists to catch non-conforming or hostile input, not to police conforming input: a decoder that hard-rejects at exactly the compressor-side limit has no margin for a peer implementation with a minor, benign spec deviation. Same footing as the compositor's total-byte budget in Devolutions#1460. 10. Multipart running-total check against the declared `uncompressedSize`, surfacing early detection of segments that collectively exceed the wire-declared bound. Unlike item 9 this one *is* spec-grounded: 2.2.5.1 defines `uncompressedSize` as the size of `segmentArray` once reassembled and decompressed, and 3.1.9.1.2.1 states it MUST equal the total number of decompressed bytes across all segments. The sender side is unchanged and already agreed with the spec: `ZGFX_SEGMENTED_MAXSIZE = 65535` in `wrapper.rs` splits into multipart above that, which is exactly the encoder-side behaviour 3.1.9.1.2.1 describes. Seven new `ZgfxError` variants total with matching Display and `Error::source` impl entries. `ZgfxError` is a public exhaustive enum, so the seven added variants (`InvalidTrailingBitCount`, `SegmentSizeExceedsBuffer`, `IncompleteBitStream`, `MatchDistanceOutOfRange`, `LengthTokenSizeTooLarge`, `SegmentDecompressedSizeExceedsLimit`, `MultipartTotalExceedsDeclared`) break exhaustive matches downstream. Confirmed by `cargo semver-checks --baseline-rev <merge-base>`: seven `enum_variant_added` failures on `ironrdp-graphics`. Title carries `!` accordingly. - `cargo xtask check fmt/lints/tests/typos/locks` all pass. - `check_egfx_zgfx_decompress` regression-replay passes against both shipped crash artifacts. - 46 existing ZGFX unit tests continue to pass against the hardened code. - Final 15-minute rigorous fuzz: 2,942,961 iterations in 901 seconds (~3,266 exec/s sustained), peak RSS 84 MB, zero panics, zero sanitizer reports, zero OOMs. Iterative audit progression on a 15-minute libFuzzer + ASan budget per round: - Round 1 (no fixes): crash at iter ~30 (trailing-bit underflow) - Round 2 (after fixes 1-2): crash at iter ~28 (bit-budget) - Round 3 (after F1-F8 audit): OOM at iter ~9877 (1.5 GB peak) - Round 4 (after F1-F10 complete): clean - Crash artifacts for the two header-parse bugs added to `crates/ironrdp-testsuite-core/test_data/fuzz_regression/egfx_zgfx_decompress/`. The later-round crashes are not added as separate regression entries because the libFuzzer corpus accumulated 161 representative inputs and the unit-tested error paths cover the cases directly. - The MS-RDPEGFX 3.1.9.1.2 ("RDP 8.0 compressor limits") per-segment bound is a semantic change: inputs that previously decoded to more than 65,535 bytes per segment now return `Err`. Such inputs were never spec-conformant. Codebase precedent (PR Devolutions#1097 et seq.) is to add `ZgfxError` variants without a `!` marker on the commit; following that convention. - Memory-budget design discussion lives on Devolutions#1120 (`issuecomment-4558356535`) rather than in this PR body so future readers find the rationale on the canonical fuzz-umbrella thread. Refs Devolutions#1316. The iteration counts and the 84 MB peak RSS were measured while the per-segment cap was 65,535. Raising it to 64 MiB does not change what the oracle exercises, but those figures describe the earlier revision rather than what ships here. Rebased onto current master (was CONFLICTING after the RDPEUDP/RDPEMT series landed). Two conflicts, both purely additive insertions colliding with unrelated new master content at the same point: - `fuzz/Cargo.toml`: master's 8 new RDPEUDP/RDPEMT `[[bin]]` entries plus this PR's own `egfx_zgfx_decompress` entry, both kept. - `crates/ironrdp-fuzzing/src/oracles/mod.rs`: master's 8 new oracle functions plus this PR's own, both appended. The automatic merge dropped this PR's own closing brace for `egfx_zgfx_decompress`, attaching the shared trailing syntax to master's last function instead of both (the same failure mode hit Devolutions#1337's rebase the other way around, master's brace dropped there). Caught before continuing the rebase this time by diffing the resolved file against both source commits directly; fixed and re-verified byte-identical to both. None of the ZGFX hardening code (`utils.rs`, `zgfx/circular_buffer.rs`, `zgfx/control_messages.rs`, `zgfx/mod.rs`) had any conflict; it applied unchanged. All checks re-run on the new head: `cargo xtask check fmt/lints/tests/typos/locks`, a direct `cargo check --all-targets` in `fuzz/`, and `cargo xtask wasm check`, all green. `fuzz/Cargo.lock` unchanged. Four findings addressed, three of them posted as review-body text rather than inline threads and never actually worked despite the PR reading as fully resolved. (1) `ZgfxError` marked `#[non_exhaustive]`, matching the same convention already on `DecodeErrorKind`/`EncodeErrorKind` in ironrdp-core; this PR adds several new variants and every downstream exhaustive match would have broken without it. (2) Corrected the doc comment on `MAX_DECOMPRESSED_PER_SEGMENT`: MS-RDPEGFX 3.1.9.1.2's compressor-limits list does supply a bound (65,535 uncompressed bytes per segment for a compliant compressor), and this crate's own compressor honors it (`wrapper.rs` panics above it, the only production caller falls back to uncompressed instead of risking it). (3) Closed a real gap: the multipart aggregate memory budget was only checked against the sender's own declared `uncompressedSize`, a fully attacker-controlled 32-bit field, so a message built from small segments could drive real allocation to roughly 4.29 GiB before that check fired. Added `MAX_DECOMPRESSED_TOTAL` (256 MiB, same footing as `MAX_COMPOSITOR_BYTES` in ironrdp-egfx's compositor.rs), checked independently. (4) Masked the trailer byte to its low 3 bits per MS-RDPEGFX 3.1.9.1.2.4 ("the five high-order bits... are reserved"); reading the whole byte let a sender's nonzero reserved bits get misread as part of the unused-bit count. Both (3) and (4) ship with regression tests negative-verified against the code without the fix. All 6 gates green throughout. Rebased onto current master, 9 behind to 0. Two conflicts, both pure unions with master's own new fuzz-target entries in `fuzz/Cargo.toml` and `fuzz_regression.rs`, same shape as the earlier rebase round. `oracles/mod.rs` auto-merged without conflict markers this time; explicitly re-checked it did not repeat the closing-brace-drop failure from that earlier round (the function's own closing brace confirmed present, then a full `cargo check -p ironrdp-fuzzing --all-targets` run clean). All 6 gates green, fuzz workspace lock stable. (cherry picked from commit 4232026) Conflict resolution (fork): upstream's context for this commit carried two sibling oracles the fork does not have — egfx_avc444_decode (from 71e0ae7) and egfx_surface_state (from 501d1eb) — which produced conflicts in crates/ironrdp-fuzzing/src/oracles/mod.rs, crates/ironrdp-testsuite-core/tests/fuzz_regression.rs and fuzz/Cargo.toml. Kept the fork's existing oracle/test/bin entries as-is and added only what 4232026 itself introduces: the egfx_zgfx_decompress oracle function and its doc-comment update to egfx_round_trip, the check_egfx_zgfx_decompress regression test, the egfx_zgfx_decompress [[bin]] entry, its fuzz target file, and its three regression test-data files. Dropped the egfx_avc444_decode oracle, test, and [[bin]] entry that appeared only as merge context from the sibling commit the fork lacks. All ZGFX hardening code in ironrdp-graphics (utils.rs, zgfx/circular_buffer.rs, zgfx/control_messages.rs, zgfx/mod.rs) applied cleanly with no conflicts. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…put (backport of Devolutions#1333, without the fuzz target) - Implements target 3 of Devolutions#1316 (egfx fuzz-coverage umbrella): `egfx_zgfx_decompress` oracle and target. - Target shape mirrors PR Devolutions#1285's `bulk_*` pattern: panic plus sanitizer oracle on `Decompressor::decompress`, fresh decompressor per iteration so history state does not leak between fuzz inputs. - Ships alongside a complete input-validation audit of the ZGFX decoder, following PR Devolutions#1271's precedent for "target plus the bugs it surfaces in one PR." Eight input-validation gaps closed in `ironrdp-graphics` (all class-(c) per Devolutions#1314: reachable via attacker-controlled wire inputs). Each one was surfaced by a rigorous smoke-fuzz iteration: 1. `Bits::try_split_to` (new in `utils.rs`) provides the checked counterpart to `split_to` that every fix below builds on. 2. `SegmentedDataPdu::from_buffer` multipart segment-size: `split_at_checked` plus a defensive `Vec::with_capacity` cap mirroring PR Devolutions#1271. 3. `decompress_segment` trailing-unused-bits subtraction: `checked_mul` + `checked_sub`. 4. `decompress_segment` token loop: checked prefix indexing + checked `split_to(8)` for the NullLiteral value. 5. `handle_match`: checked distance-bits split, `checked_add` for `distance_base + value`, plus `distance > HISTORY_SIZE` bound check. 6. `read_unencoded_bytes`: checked splits for the 15-bit length, pad-to-boundary, and `length * 8` payload. 7. `read_encoded_bytes`: checked splits + `usize::BITS` bound on `load_be::<usize>()` + `checked_shl` replacing `pow` + `checked_add` for `base + value`. 8. `FixedCircularBuffer::read_with_offset`: defense-in-depth `offset > buffer.len()` bound check below the caller-side guard. Plus memory-budget enforcement, which a match-copy chain makes necessary: it can expand a few bytes of input into unbounded output, and an unbounded run reached a 1.5 GB peak. 9. New `MAX_DECOMPRESSED_PER_SEGMENT = 64 MiB` ceiling on a single segment's decompressed output, enforced per-token in `decompress_segment` and threaded through the match-copy paths so it is checked before any allocation. **This is an implementation resource limit, not a wire requirement**, and the code says so, but MS-RDPEGFX 3.1.9.1.2 ("RDP 8.0 compressor limits") does supply a number: a compliant compressor MUST NOT produce any single segment past 65,535 uncompressed bytes. This crate's production compressor path honors that: `wrapper.rs`'s `wrap_compressed` panics above 65,535, and `compress_and_wrap_egfx`, the only production caller, falls back to uncompressed rather than risk exceeding it. The `compress_high_entropy_round_trips_and_bounds_table` test that an earlier revision of this PR body cited as evidence real traffic exceeds 65,535 does not show that: it calls `Compressor::compress` directly and feeds the result straight to `decompress_segment`, bypassing the wrapper every real sender uses. 64 MiB is still used here, not 65,535, because this ceiling exists to catch non-conforming or hostile input, not to police conforming input: a decoder that hard-rejects at exactly the compressor-side limit has no margin for a peer implementation with a minor, benign spec deviation. Same footing as the compositor's total-byte budget in Devolutions#1460. 10. Multipart running-total check against the declared `uncompressedSize`, surfacing early detection of segments that collectively exceed the wire-declared bound. Unlike item 9 this one *is* spec-grounded: 2.2.5.1 defines `uncompressedSize` as the size of `segmentArray` once reassembled and decompressed, and 3.1.9.1.2.1 states it MUST equal the total number of decompressed bytes across all segments. The sender side is unchanged and already agreed with the spec: `ZGFX_SEGMENTED_MAXSIZE = 65535` in `wrapper.rs` splits into multipart above that, which is exactly the encoder-side behaviour 3.1.9.1.2.1 describes. Seven new `ZgfxError` variants total with matching Display and `Error::source` impl entries. `ZgfxError` is a public exhaustive enum, so the seven added variants (`InvalidTrailingBitCount`, `SegmentSizeExceedsBuffer`, `IncompleteBitStream`, `MatchDistanceOutOfRange`, `LengthTokenSizeTooLarge`, `SegmentDecompressedSizeExceedsLimit`, `MultipartTotalExceedsDeclared`) break exhaustive matches downstream. Confirmed by `cargo semver-checks --baseline-rev <merge-base>`: seven `enum_variant_added` failures on `ironrdp-graphics`. Title carries `!` accordingly. - `cargo xtask check fmt/lints/tests/typos/locks` all pass. - `check_egfx_zgfx_decompress` regression-replay passes against both shipped crash artifacts. - 46 existing ZGFX unit tests continue to pass against the hardened code. - Final 15-minute rigorous fuzz: 2,942,961 iterations in 901 seconds (~3,266 exec/s sustained), peak RSS 84 MB, zero panics, zero sanitizer reports, zero OOMs. Iterative audit progression on a 15-minute libFuzzer + ASan budget per round: - Round 1 (no fixes): crash at iter ~30 (trailing-bit underflow) - Round 2 (after fixes 1-2): crash at iter ~28 (bit-budget) - Round 3 (after F1-F8 audit): OOM at iter ~9877 (1.5 GB peak) - Round 4 (after F1-F10 complete): clean - Crash artifacts for the two header-parse bugs added to `crates/ironrdp-testsuite-core/test_data/fuzz_regression/egfx_zgfx_decompress/`. The later-round crashes are not added as separate regression entries because the libFuzzer corpus accumulated 161 representative inputs and the unit-tested error paths cover the cases directly. - The MS-RDPEGFX 3.1.9.1.2 ("RDP 8.0 compressor limits") per-segment bound is a semantic change: inputs that previously decoded to more than 65,535 bytes per segment now return `Err`. Such inputs were never spec-conformant. Codebase precedent (PR Devolutions#1097 et seq.) is to add `ZgfxError` variants without a `!` marker on the commit; following that convention. - Memory-budget design discussion lives on Devolutions#1120 (`issuecomment-4558356535`) rather than in this PR body so future readers find the rationale on the canonical fuzz-umbrella thread. Refs Devolutions#1316. The iteration counts and the 84 MB peak RSS were measured while the per-segment cap was 65,535. Raising it to 64 MiB does not change what the oracle exercises, but those figures describe the earlier revision rather than what ships here. Rebased onto current master (was CONFLICTING after the RDPEUDP/RDPEMT series landed). Two conflicts, both purely additive insertions colliding with unrelated new master content at the same point: - `fuzz/Cargo.toml`: master's 8 new RDPEUDP/RDPEMT `[[bin]]` entries plus this PR's own `egfx_zgfx_decompress` entry, both kept. - `crates/ironrdp-fuzzing/src/oracles/mod.rs`: master's 8 new oracle functions plus this PR's own, both appended. The automatic merge dropped this PR's own closing brace for `egfx_zgfx_decompress`, attaching the shared trailing syntax to master's last function instead of both (the same failure mode hit Devolutions#1337's rebase the other way around, master's brace dropped there). Caught before continuing the rebase this time by diffing the resolved file against both source commits directly; fixed and re-verified byte-identical to both. None of the ZGFX hardening code (`utils.rs`, `zgfx/circular_buffer.rs`, `zgfx/control_messages.rs`, `zgfx/mod.rs`) had any conflict; it applied unchanged. All checks re-run on the new head: `cargo xtask check fmt/lints/tests/typos/locks`, a direct `cargo check --all-targets` in `fuzz/`, and `cargo xtask wasm check`, all green. `fuzz/Cargo.lock` unchanged. Four findings addressed, three of them posted as review-body text rather than inline threads and never actually worked despite the PR reading as fully resolved. (1) `ZgfxError` marked `#[non_exhaustive]`, matching the same convention already on `DecodeErrorKind`/`EncodeErrorKind` in ironrdp-core; this PR adds several new variants and every downstream exhaustive match would have broken without it. (2) Corrected the doc comment on `MAX_DECOMPRESSED_PER_SEGMENT`: MS-RDPEGFX 3.1.9.1.2's compressor-limits list does supply a bound (65,535 uncompressed bytes per segment for a compliant compressor), and this crate's own compressor honors it (`wrapper.rs` panics above it, the only production caller falls back to uncompressed instead of risking it). (3) Closed a real gap: the multipart aggregate memory budget was only checked against the sender's own declared `uncompressedSize`, a fully attacker-controlled 32-bit field, so a message built from small segments could drive real allocation to roughly 4.29 GiB before that check fired. Added `MAX_DECOMPRESSED_TOTAL` (256 MiB, same footing as `MAX_COMPOSITOR_BYTES` in ironrdp-egfx's compositor.rs), checked independently. (4) Masked the trailer byte to its low 3 bits per MS-RDPEGFX 3.1.9.1.2.4 ("the five high-order bits... are reserved"); reading the whole byte let a sender's nonzero reserved bits get misread as part of the unused-bit count. Both (3) and (4) ship with regression tests negative-verified against the code without the fix. All 6 gates green throughout. Rebased onto current master, 9 behind to 0. Two conflicts, both pure unions with master's own new fuzz-target entries in `fuzz/Cargo.toml` and `fuzz_regression.rs`, same shape as the earlier rebase round. `oracles/mod.rs` auto-merged without conflict markers this time; explicitly re-checked it did not repeat the closing-brace-drop failure from that earlier round (the function's own closing brace confirmed present, then a full `cargo check -p ironrdp-fuzzing --all-targets` run clean). All 6 gates green, fuzz workspace lock stable. (cherry picked from commit 4232026)
Summary
egfx_surface_statestate-machine oracle. Sibling ofegfx_multi_frame(PR feat(fuzz): add egfx_multi_frame oracle and target #1336) with two distinct properties.SurfaceLifecyclePduenum containing the seven variants relevant to surface lifecycle (ResetGraphics,CreateSurface,DeleteSurface,MapSurfaceToOutput,StartFrame,EndFrame,FrameAcknowledge), concentrating fuzz pressure on the surface and frame state machine rather than spreading across all 23GfxPduvariants. Two of the seven do not themselves affect modeled state:FrameAcknowledgehas no arm in the client's receive path, included as legitimate hostile-server input;StartFrameis unmodeled since its state lives in private fields with no public getter.ExpectedStatestruct mirrors the client's documented per-surface fields (id,width,height,pixel_format,is_mapped,output_origin_x/y) plus thetotal_frames_decodedcounter. For each successfully-encoded PDU the model is updated to mirror the client'shandle_*transitions; after dispatch the client's observable state via the publicget_surfaceandtotal_frames_decodedgetters is asserted against the model.What this catches
A different bug class from
egfx_multi_frame's panic / sanitizer oracle: logic bugs in state transitions that produce wrong but non-crashing observable state. Examples:DeleteSurface(model removed it, client did not).is_mappedon a surface that did not receiveMapSurfaceToOutput.ResetGraphics.total_frames_decodedon a PDU other thanEndFrame.The model encodes the implementation's current documented behavior (e.g.,
CreateSurfacewithwidth == 0orheight == 0is silently skipped perhandle_create_surface,total_frames_decodedis incremented unconditionally byhandle_end_frameeven when no matchingStartFramepreceded it). The oracle catches drift from that behavior. Spec-compliance verification is a separate concern.What this does NOT catch
State transitions that exist only in private fields without a public getter (
current_frame_id,frames_queued). Wiring observable access to those fields is a separate egfx-public-API change, out of scope here.Dependencies
This PR is stacked on PR #1336 (
egfx_multi_frame, target 5) and must merge after it. TheArbitrarycascade both harnesses need landed in #1334, so #1336 is the only remaining ancestor. Both branches are rebased onto current master and green.Validation
cargo xtask check fmt/lints/tests/typos/locksall pass.check_egfx_surface_stateregression-replay test passes against the seed-empty corpus entry.Notes
SurfaceLifecyclePduenum lives oracle-private inside the function (not exported fromironrdp-egfx), keeping the egfx public surface clean per the fallback shape from the design discussion on egfx: extend fuzz coverage to OpenH264-wrapper, ZGFX, multi-frame state, and encoder round-trip #1316.as _imports forArbitraryandDvcProcessorper clippy'sunused_trait_nameslint, matching theegfx_multi_framestyle.#[expect(clippy::panic, reason = ...)]and# Panicsdoc section match the precedent frombulk_round_tripfor assertion-based oracles.With this PR the #1316 umbrella has its initial six target slots filled (1 merged, 2+2b+3+5+6 open).
Refs #1316. Depends on #1336.
Rebase 2026-08-19
Rebased onto current master (was CONFLICTING after the RDPEUDP/RDPEMT series landed). Two conflicts, both purely additive insertions at the same point as unrelated new content master added:
fuzz/Cargo.toml: master added 8 new[[bin]]entries for the RDPEUDP/RDPEMT fuzz targets at the same list position as this PR's ownegfx_surface_stateentry. Resolved by keeping both sets.crates/ironrdp-fuzzing/src/oracles/mod.rs: same shape, master's 8 new oracle functions plus this PR's ownegfx_surface_stateoracle, both appended at end of file. The automatic merge here needed a manual fix: it dropped the closing braces of master's last function (rdpeudp_ack_vector's oracle), since git's diff3 alignment treated that closing syntax as shared trailing context and attached it only to this PR's block instead of both. Caught by diffing the resolved file against both source commits directly rather than trusting the merge; fixed, then re-verified byte-identical to both sources.No logic in the oracle or the state model changed, so the fuzz run and validation numbers above still describe the actual code; only the commit SHAs changed (rebase, no content edits beyond the two conflict resolutions). All checks re-run on the new head:
cargo xtask check fmt/lints/tests/typos/locks, a directcargo check --all-targetsin thefuzz/workspace (not covered by the main gate), andcargo xtask wasm check, all green.fuzz/Cargo.lockunchanged.