feat(fuzz): add egfx PDU decoders to pdu_decode oracle + fix two surfaced bugs#1271
Open
Greg Lamberson (glamberson) wants to merge 3 commits into
Open
Conversation
Member
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you! That was a blind spot
Greg Lamberson (glamberson)
force-pushed
the
feat/ironrdp-egfx-fuzz-target
branch
from
962182a to
c5211e4
Compare
Greg Lamberson (glamberson)
changed the title
Adds eight egfx decode calls to the existing `pdu_decode` oracle: `GfxPdu`, `CacheToSurfacePdu`, `CapabilitySet`, `Avc420BitmapStream`, `Avc444BitmapStream`, `QuantQuality`, `Point`, `Color`. Folds into `pdu_decode` rather than a separate target per maintainer suggestion, since the byte-stream oracle pattern is identical. `ironrdp-egfx` was the only Core-Tier-by-behaviour crate without dedicated fuzz coverage of its PDU decoders.
The Avc444BitmapStream decoder reads a 30-bit stream length from the header and then calls src.split_at(stream_len as usize). ReadCursor's split_at panics when the requested offset exceeds the remaining buffer, which crashes the decoder on malformed input where the declared streamLen field is larger than the actual payload. Adds an ensure_size! check before split_at so the same condition returns a NotEnoughBytes error through DecodeResult instead of panicking. Surfaced by the egfx fuzz coverage added in the previous commit on this branch.
Avc420BitmapStream::decode reads a u32 num_regions and called Vec::with_capacity(num_regions as usize) twice without bounding the value. A fuzzer-controlled num_regions near u32::MAX caused the allocator to attempt several gigabytes per call, which ASan trapped as out-of-memory in the new pdu_decode egfx coverage. Caps the with_capacity argument at src.len() / per-region size, where per-region is the minimum decode footprint (InclusiveRectangle plus QuantQuality). The actual read loop is unchanged and still returns NotEnoughBytes via the inner Decode impls if num_regions does not match the actual payload, so the only behavioural change is bounded pre-allocation. Surfaced by the egfx fuzz coverage added earlier on this branch.
Greg Lamberson (glamberson)
force-pushed
the
feat/ironrdp-egfx-fuzz-target
branch
from
f741fb2 to
1d11c95
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends the existing
pdu_decodeoracle inironrdp-fuzzing::oracleswith EGFX coverage, satisfying the Core Tier "must be fuzzed" invariant (perARCHITECTURE.md) for the graphics-pipeline DVC parsing. Two pre-existing bugs inironrdp-egfx'sAvc444BitmapStream/Avc420BitmapStreamdecoders that the new coverage immediately surfaced are fixed in the same series.What the oracle exercises
The EGFX decode list added to
pdu_decoderuns the input through:GfxPdu(umbrella enum covering all 23 EGFX message types — primary entry point)CacheToSurfacePdu(sole sub-PDU with an independentDecodeimpl)EgfxCapabilitySet(aliased to disambiguate againstcapability_sets::CapabilitySetfromironrdp-pdu)Avc420BitmapStream<'_>andAvc444BitmapStream<'_>QuantQuality,Point,ColorShape
Folded into the existing
pdu_decodeoracle inc5211e46per your suggestion on the original filing ("I think it's fine to add this directly topdu_decode, so we don't have an extra target for essentially the same logic"). No separateegfx_pdu_decoding.rstarget /[[bin]]entry; the EGFX decoders sit alongside the existing PDU decoders in the same oracle and share the corpus.Bug fixes surfaced by the new coverage
Both pre-existing in
ironrdp-egfx; surfaced by the EGFX decode additions before this PR could merge cleanly.f96ba08e fix(egfx): bound stream_len before split_at in Avc444BitmapStream:Avc444BitmapStream::decoderead a 30-bitstreamLenfield and calledsrc.split_at(stream_len as usize)without bounds-checking.ReadCursor::split_atpanics on offset overflow. Fixed withensure_size!(ctx: Self::NAME, in: src, size: stream_len)before the split, so a malformedstreamLenreturnsNotEnoughBytesinstead of panicking.f741fb26 fix(egfx): cap Avc420BitmapStream pre-allocation against remaining bytes:Avc420BitmapStream::decoderead a u32num_regionsand calledVec::with_capacity(num_regions as usize)twice without bounding. ASan tripped on a ~33 GB allocation fornum_regions = 0xF7_00_AC_7E. Fixed by cappingwith_capacityatsrc.len() / (InclusiveRectangle::FIXED_PART_SIZE + QuantQuality::FIXED_PART_SIZE), matching the bounded-pre-allocation pattern incliprdr/format_data/file_list.rs. The actual decode loop is unchanged.Verification
cargo xtask check fmt✓cargo xtask check lints✓cargo xtask check locks✓cargo xtask check typos✓cargo xtask check tests✓cargo check --manifest-path fuzz/Cargo.tomlbuilds thepdu_decodingtarget cleanlyf741fb26: all 9 jobs green including Fuzzing 5m21s (the same 5-minute fuzz budget that previously surfaced both bugs now passes clean)Refs #1120.