test(fuzz): assert the round-trip oracle can re-decode what it encodes - #1492
Merged
Marc-André Moreau (mamoreau-devolutions) merged 1 commit intoAug 3, 2026
Conversation
Greg Lamberson (glamberson)
force-pushed
the
fuzz/round-trip-assert-redecode
branch
from
July 31, 2026 19:11
6f35831 to
e6fc372
Compare
`pdu_round_trip_one!` discarded the result of the re-decode, so an encoder that emitted bytes it could not read back passed silently. The doc comment said such asymmetries were "tracked via filed follow-up issues"; in practice they were not, and the Bandwidth Measure Stop asymmetry fixed earlier in this stack went unnoticed until it was hit by hand while writing an unrelated test. A successful encode is now asserted to be re-decodable. A failing decode of the fuzzer's input is still skipped, and a failing encode is still tolerated, since several types return "Encoding not implemented" for variants the decoder accepts. Byte stability across the round trip is deliberately not asserted. Several decoders normalise: `LogonInfoVersion1` range-checks `domainNameSize` and then keeps only the trimmed string, so a PDU whose size field disagrees with its own padding cannot re-encode identically however correct both halves are. Asserting that reported design as breakage. Auto-detect PDUs are added to the type list. Their absence is the other reason the Bandwidth Measure Stop asymmetry survived. Verified: 3.5M runs clean; reverting either fix in this stack reproduces a failure within 45 seconds.
Greg Lamberson (glamberson)
force-pushed
the
fuzz/round-trip-assert-redecode
branch
from
August 3, 2026 01:09
e6fc372 to
a524437
Compare
Marc-André Moreau (mamoreau-devolutions)
approved these changes
Aug 3, 2026
Marc-André Moreau (mamoreau-devolutions)
merged commit Aug 3, 2026
f6e4d68
into
Devolutions:master
26 checks passed
Marc-André Moreau (mamoreau-devolutions)
pushed a commit
that referenced
this pull request
Aug 3, 2026
…de (#1511) ## What A connect-time Bandwidth Measure Stop with `payloadLength` of zero now decodes, as a present-but-empty payload. `Encode` still refuses to emit one. ## Why [MS-RDPBCGR] 2.2.14.1.4 says of `payloadLength`: "It MUST be present (and have a value greater than zero) if the value of the **requestType** field is set to 0x002B." #1491 read that as a rule for both directions and made encode and decode refuse a zero. The encode half is right. The decode half is not. The two directions answer different questions. Encoding asks what we are permitted to put on the wire, and a zero length has no conforming encoding, so refusing is correct. Decoding asks whether we can act on what a peer already sent. Here we can: `sequenceNumber` and `requestType` arrive intact, and those fully determine the Bandwidth Measure Results reply the PDU is asking for. The payload is random measurement filler per the same section, and its length is the only thing the reply reports about it. Rejecting therefore discards a PDU we could have answered without gaining any protection. FreeRDP-based servers, including gnome-remote-desktop, block in `AWAIT_BW_RESULT` until the results arrive, so a server that sends a zero length stalls the whole connection rather than getting a diagnostic. The fix #1491 was actually titled for, keying the optional fields off `requestType` instead of the `Option`, is untouched. Only the added zero-length rejection moves, and only on the receive side. ## Tests `connect_time_stop_with_a_zero_payload_length_is_rejected` becomes `..._is_accepted` and now asserts the decoded value: `payload: Some(vec![])`, present and empty rather than absent, since the wire carried a length field. Added `a_decoded_zero_length_stop_does_not_re_encode`, so the asymmetry is stated as a test rather than only as a comment. `connect_time_stop_without_a_payload_is_refused` is unchanged and still covers the encode side. ## Note This does not break the `pdu_round_trip` oracle in #1492: a failing `encode` after a successful `decode` is already tolerated there, and the re-decode assertion only fires on a successful encode. ## Verification `cargo xtask check fmt/lints/tests/typos/locks` all pass. `cargo semver-checks` reports no update required for `ironrdp-pdu`. ## Unblocks #1465 #1465 carries a regression test for a zero-`payloadLength` Stop, which cannot pass until this lands: #1491 made `AutoDetectRequest`'s decoder reject `payloadLength = 0`, so such a PDU is refused before it reaches the connector. Its `connect_time_bandwidth_answers_a_stop_carrying_an_empty_payload` is red today and that red is this dependency, not a defect there. Verified against current `master`: #1465 alone fails that one case out of 1032; #1465 with this applied passes all of them. Merging this first turns #1465 green with no change on its side. ## Rebased Rebased onto `master` on 2026-08-02 so the checks run against the current tree rather than the state before that day's merges. No conflicts, no content change.
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
The
pdu_round_triporacle discarded the result of its own re-decode, so an encoderthat emitted bytes it could not read back passed silently. This asserts it, fixes the
one pre-existing failure that turning it on exposes, and adds the auto-detect PDUs that
were missing from the type list.
Stacked on #1491. That PR fixes the
BandwidthMeasureStopasymmetry which this oraclewould otherwise fail on immediately, so it needs to land first.
Why the assertion was missing, and why that did not work
The macro ended in
let _ = decode(&encoded);. The doc comment was explicit that thiswas deliberate:
In practice they were not tracked. The
BandwidthMeasureStopasymmetry in #1491 wentunnoticed and was found by hand, while writing an unrelated test. Emitting bytes we
cannot read back is a real defect on the wire, so it is asserted now.
What turning it on found
LogonInfoVersion1::encodeclamps both names to their fixed-width fields withresize,then wrote the untruncated length into
domainNameSizeanduserNameSize. For a nameat the field boundary that advertises a size larger than the field, and this crate's own
decoder rejects
domainNameSize > DOMAIN_NAME_SIZE_V1, so the encoder could produce aPDU it could not read back. Fixed in the first commit here: the sizes now describe what
is actually emitted.
Found in under a minute of fuzzing.
What is deliberately not asserted
An earlier revision also asserted byte stability, that decode-then-encode reproduces the
input bytes. That is not a valid property here and the fuzzer said so within a minute.
Several decoders normalise.
LogonInfoVersion1readsdomainNameSize, range-checks it,and then keeps only the trimmed string; the size field is not represented in
LogonInfo. A PDU whose size field disagrees with its own zero padding therefore cannotre-encode to identical bytes however correct both halves are. Asserting it reported
design as breakage, so it was removed. Only re-decodability is asserted.
Auto-detect coverage
AutoDetectReqPduandAutoDetectRspPduwere absent from the type list. Their absenceis the other half of why the #1491 asymmetry survived: even with the assertion, nothing
was feeding those types through the oracle.
Verification
cargo xtask check fmt -vgreencargo xtask check lints -vgreencargo xtask check tests -vgreencargo xtask check typos -vgreencargo xtask check locks -vgreencargo +nightly check --lockedinfuzz/green (that workspace's lock is not coveredby
xtask check locks)