Skip to content

feat(rdpsnd)!: misuse-resistant format negotiation for RdpsndServerHandler#1359

Open
clintcan wants to merge 1 commit into
Devolutions:masterfrom
clintcan:feat-rdpsnd-negotiated-format
Open

feat(rdpsnd)!: misuse-resistant format negotiation for RdpsndServerHandler#1359
clintcan wants to merge 1 commit into
Devolutions:masterfrom
clintcan:feat-rdpsnd-negotiated-format

Conversation

@clintcan
Copy link
Copy Markdown
Contributor

@clintcan clintcan commented Jun 2, 2026

Implements the misuse-resistant RdpsndServerHandler format-negotiation API discussed in #1356, in the two-method shape you proposed there.

Problem

The old start(&ClientAudioFormatPdu) -> Option<u16> made every handler compute wFormatNo itself — an index into the client's format list. Returning the wrong index (e.g. a server-list index) yields wFormatNo >= NumberOfClientFormats, which a compliant client rejects, silently dropping all audio. Every handler also re-implemented the same server∩client intersection. (Documented in #1343.)

Change

Move the negotiation into the crate and split selection from lifecycle:

fn choose_format<'a>(&mut self, common: &'a [NegotiatedFormat]) -> Option<&'a NegotiatedFormat>;
fn start(&mut self, format: &NegotiatedFormat);
  • The crate computes common — the formats from get_formats() the client also advertised, in the server's preference order, each tagged with its client-list wFormatNo — and calls choose_format, then start with the chosen format.
  • NegotiatedFormat has a private wformat_no and no public constructor, and choose_format returns a borrow of an element of common, so a handler cannot pick a format the client didn't accept nor emit an out-of-range wFormatNo.
  • choose_format is not called when nothing is in common.
  • choose_format (pure selection) and start (encoder init / producer startup) are kept distinct, per your note that start is also where producers initialize.

Resolves the footgun in #1343. Closes #1356.

Two decisions worth a look (easy to change)

  1. NegotiatedFormat.format is private with a format() accessor rather than pub formatclippy::partial_pub_fields rejects mixing a pub field with the private wformat_no, and this matches the direction of fix(egfx)!: make DecodedFrame fields private with getters to enforce size invariant #1331. Happy to switch to pub + #[allow] if you prefer.
  2. Format matching uses WAVEFORMATEX identity (audio_format_eq: wave-format tag, channels, sample rate, bit depth), deliberately ignoring derived fields (n_avg_bytes_per_sec, n_block_align) and the codec data blob, which a client may not echo byte-for-byte. This is looser than the type's derived ==. I went with identity matching because strict equality can miss formats a client genuinely supports (→ silent no-audio, the very bug this targets), but if you'd rather the crate use full ==, it's a one-line change.

Notes

Testing

  • New unit tests for negotiate_formats / audio_format_eq: client-index mapping, the PCM-only/AAC-first regression, empty overlap, and identity equality. (Enabled the lib test harness in Cargo.toml.)
  • cargo clippy -p ironrdp-rdpsnd --all-targets -- -D warnings and the example (--features cliprdr,connector,rdpsnd,server) are clean; cargo test -p ironrdp-rdpsnd passes.

…ndler

The old `start(&ClientAudioFormatPdu) -> Option<u16>` made every handler
compute `wFormatNo` itself, as an index into the *client's* format list.
Getting it wrong (e.g. returning a server-list index) yields
`wFormatNo >= NumberOfClientFormats`, which a compliant client rejects,
silently dropping all audio — and each handler re-implemented the same
server-vs-client intersection.

Move that work into the crate and split selection from lifecycle:

    fn choose_format<'a>(&mut self, common: &'a [NegotiatedFormat]) -> Option<&'a NegotiatedFormat>;
    fn start(&mut self, format: &NegotiatedFormat);

The crate computes `common` (formats from `get_formats()` the client also
advertised, in the server's preference order, each tagged with its
client-list `wFormatNo`), calls `choose_format`, then `start` with the
chosen format. `NegotiatedFormat` has a private `wformat_no` and no public
constructor, and `choose_format` returns a borrow of an element of `common`,
so a handler cannot pick a format the client did not accept nor emit an
out-of-range `wFormatNo`. `choose_format` is not called when nothing is in
common. Separating `choose_format` (pure selection) from `start` (encoder
init / producer startup) keeps the two concerns distinct.

Resolves the footgun documented in Devolutions#1343. Closes Devolutions#1356.

BREAKING CHANGE: `RdpsndServerHandler::start` is replaced by `choose_format`
(selection) plus `start(&NegotiatedFormat)` (lifecycle). Implementors now
return a `&NegotiatedFormat` from `choose_format` instead of computing a
`wFormatNo`.

- Add `NegotiatedFormat` (private `wformat_no`, `format()` accessor) and
  `negotiate_formats` (intersection + client-index mapping), with unit tests
  for client-index mapping, the PCM-only/AAC-first regression, empty overlap,
  and WAVEFORMATEX-identity equality.
- Match formats on WAVEFORMATEX identity (tag/channels/rate/bits), ignoring
  derived and codec-`data` fields a client may not echo verbatim.
- Update the example server to the new two-method shape.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

rdpsnd: make RdpsndServerHandler::start misuse-resistant (hand it the common formats, let the crate compute wFormatNo)

1 participant