refactor(pdu): make the CapabilitySet encoder exhaustive#1328
Open
Greg Lamberson (glamberson) wants to merge 1 commit into
Open
refactor(pdu): make the CapabilitySet encoder exhaustive#1328Greg Lamberson (glamberson) wants to merge 1 commit into
Greg Lamberson (glamberson) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR is part of the ironrdp-pdu audit work (issue #1314) to remove runtime unreachable!() catch-alls in capability-set code paths and replace them with structures that either become compile-time exhaustive (for enum variants) or remove provably-dead branches (for GUID-discriminated decoding). The goal is to turn future regressions from runtime panics into compile errors or eliminate unreachable branches entirely.
Changes:
- In
CapabilitySet::encode, replaced an inner_ => unreachable!()with an explicit list of the structuredCapabilitySetvariants, so adding a new enum variant forces this site to be updated at compile time. - In
Codecdecoding forGUID_REMOTEFX | GUID_IMAGE_REMOTEFX, removed a dead nestedmatch ... _ => unreachable!()and replaced it with anif/elsebased on the already-constrainedguid.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/ironrdp-pdu/src/rdp/capability_sets/mod.rs | Makes the raw-buffer fallback’s inner match exhaustive w.r.t. CapabilitySet variants, preventing future missing-variant bugs from compiling. |
| crates/ironrdp-pdu/src/rdp/capability_sets/bitmap_codecs/mod.rs | Removes an unreachable nested match arm in RemoteFX property decoding by leveraging the outer match’s constraint on guid. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The inner match in `CapabilitySet::Encode::encode`'s raw-buffer fallback (`mod.rs:448`) used a `_ => unreachable!()` catch-all. Replace it with an explicit enumeration of the 17 structured `CapabilitySet` variants already handled by the outer match's specific arms, so adding a new variant to `CapabilitySet` is a compile error here until the variant is routed in this `Encode` impl. The arm body stays `unreachable!()` because it is genuinely unreachable under the current outer-match structure. PR Devolutions#1313 (BitmapCacheV3 encoder `unreachable!()` reached on decoder-accepted input) demonstrated why a runtime catch-all is the wrong shape for this match. The nested `match guid { ... _ => unreachable!() }` in `CodecProperty` decode for `GUID_REMOTEFX | GUID_IMAGE_REMOTEFX` (`bitmap_codecs/mod.rs`) keeps its structure. Per review, `unreachable!()` is the right tool there: `guid` is already validated by the outer arm, so the `_` branch is a redundant correctness check that fires under tests and fuzzing if a future change breaks the invariant. The only change is a message documenting that invariant inline. This is part of the audit pass on issue Devolutions#1314. Both sites are class (a) (provably unreachable in current code), confirmed by `pdu_round_trip` (Devolutions#1291) and `message_decoding_invariants` (Devolutions#1320) exercising `CapabilitySet` and `ShareControlHeader` for millions of iterations post-Devolutions#1313 with no panics. Refs Devolutions#1314.
Greg Lamberson (glamberson)
force-pushed
the
refactor/capability-sets-exhaustive-encoder
branch
from
ba67cc7 to
6b00318
Compare
Greg Lamberson (glamberson)
changed the title
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.
Part of the
unreachable!()/panic!()audit pass on #1314, scoped tocrates/ironrdp-pdu/src/rdp/capability_sets/.mod.rs(CapabilitySet::Encode::encoderaw-buffer fallback): the inner_ => unreachable!()catch-all is replaced with an explicit enumeration of the 17 structuredCapabilitySetvariants already handled by the outer match. Adding a new variant toCapabilitySetis now a compile error here until it is routed in thisEncodeimpl. The arm body staysunreachable!()(genuinely unreachable under the current outer-match structure). PR #1313 (BitmapCacheV3 encoderunreachable!()reached on decoder-accepted input) is why a runtime catch-all is the wrong shape here.bitmap_codecs/mod.rs(CodecPropertydecode forGUID_REMOTEFX | GUID_IMAGE_REMOTEFX): keeps itsmatch guid { ... _ => unreachable!() }structure per review.guidis already validated by the outer arm, so the_branch is a redundant correctness check that fires under tests and fuzzing if a future change breaks the invariant. The only change is an inline message documenting that invariant.Both sites are class (a) (provably unreachable in current code), confirmed by
pdu_round_trip(#1291) andmessage_decoding_invariants(#1320) exercisingCapabilitySetandShareControlHeaderfor millions of iterations post-#1313 with no panics.Refs #1314.