Skip to content

test(fuzz): assert the round-trip oracle can re-decode what it encodes - #1492

Merged
Marc-André Moreau (mamoreau-devolutions) merged 1 commit into
Devolutions:masterfrom
lamco-admin:fuzz/round-trip-assert-redecode
Aug 3, 2026
Merged

test(fuzz): assert the round-trip oracle can re-decode what it encodes#1492
Marc-André Moreau (mamoreau-devolutions) merged 1 commit into
Devolutions:masterfrom
lamco-admin:fuzz/round-trip-assert-redecode

Conversation

@glamberson

Copy link
Copy Markdown
Contributor

Summary

The pdu_round_trip oracle discarded the result of its own re-decode, so an encoder
that 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 BandwidthMeasureStop asymmetry which this oracle
would 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 this
was deliberate:

Re-decode returning Err. Surfaces an asymmetry but not a memory-safety bug; tracked
via filed follow-up issues, not this oracle.

In practice they were not tracked. The BandwidthMeasureStop asymmetry in #1491 went
unnoticed 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::encode clamps both names to their fixed-width fields with resize,
then wrote the untruncated length into domainNameSize and userNameSize. For a name
at 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 a
PDU 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. LogonInfoVersion1 reads domainNameSize, 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 cannot
re-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

AutoDetectReqPdu and AutoDetectRspPdu were absent from the type list. Their absence
is the other half of why the #1491 asymmetry survived: even with the assertion, nothing
was feeding those types through the oracle.

Verification

  • 3.5M runs clean with the stack applied
  • reverting either fix reproduces a failure within 45 seconds, so the assertion bites
  • cargo xtask check fmt -v green
  • cargo xtask check lints -v green
  • cargo xtask check tests -v green
  • cargo xtask check typos -v green
  • cargo xtask check locks -v green
  • cargo +nightly check --locked in fuzz/ green (that workspace's lock is not covered
    by xtask check locks)

@github-actions github-actions Bot added origin/fuzzing Crashes and other bugs found by fuzzing scope/core Touches the core architectural tier A-internal size/M Size: 150-399 lines of code labels Jul 31, 2026
@glamberson
Greg Lamberson (glamberson) force-pushed the fuzz/round-trip-assert-redecode branch from 6f35831 to e6fc372 Compare July 31, 2026 19:11
@github-actions github-actions Bot added size/M Size: 150-399 lines of code and removed size/M Size: 150-399 lines of code labels Jul 31, 2026
`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.
@glamberson
Greg Lamberson (glamberson) force-pushed the fuzz/round-trip-assert-redecode branch from e6fc372 to a524437 Compare August 3, 2026 01:09
@github-actions github-actions Bot added size/S Size: 30-149 lines of code and removed scope/core Touches the core architectural tier size/M Size: 150-399 lines of code labels Aug 3, 2026
@mamoreau-devolutions
Marc-André Moreau (mamoreau-devolutions) merged commit f6e4d68 into Devolutions:master Aug 3, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

origin/fuzzing Crashes and other bugs found by fuzzing size/S Size: 30-149 lines of code

Development

Successfully merging this pull request may close these issues.

2 participants