feat(rdpsnd)!: misuse-resistant format negotiation for RdpsndServerHandler#1359
feat(rdpsnd)!: misuse-resistant format negotiation for RdpsndServerHandler#1359clintcan wants to merge 1 commit into
Conversation
…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.
f6e94a5 to
72d28d2
Compare
|
Friendly ping on this when you have a moment 🙂 — it's been a few weeks and I wanted to make sure it didn't slip off the radar. This implements the exact two-method shape we settled on in #1356: Two small in-PR points I'd flagged for your call are still open whenever you review: (1) the |
Implements the misuse-resistant
RdpsndServerHandlerformat-negotiation API discussed in #1356, in the two-method shape you proposed there.Problem
The old
start(&ClientAudioFormatPdu) -> Option<u16>made every handler computewFormatNoitself — an index into the client's format list. Returning the wrong index (e.g. a server-list index) yieldswFormatNo >= 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:
common— the formats fromget_formats()the client also advertised, in the server's preference order, each tagged with its client-listwFormatNo— and callschoose_format, thenstartwith the chosen format.NegotiatedFormathas a privatewformat_noand no public constructor, andchoose_formatreturns a borrow of an element ofcommon, so a handler cannot pick a format the client didn't accept nor emit an out-of-rangewFormatNo.choose_formatis not called when nothing is in common.choose_format(pure selection) andstart(encoder init / producer startup) are kept distinct, per your note thatstartis also where producers initialize.Resolves the footgun in #1343. Closes #1356.
Two decisions worth a look (easy to change)
NegotiatedFormat.formatis private with aformat()accessor rather thanpub format—clippy::partial_pub_fieldsrejects mixing apubfield with the privatewformat_no, and this matches the direction of fix(egfx)!: make DecodedFrame fields private with getters to enforce size invariant #1331. Happy to switch topub+#[allow]if you prefer.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 codecdatablob, 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
RdpsndServerHandler::start. Known downstream implementors that will need a small update:lamco-rdp-server,hypr-rdp,cosmic-ext-rdp-server,ARISU,macrdp. (Per the rdpsnd: make RdpsndServerHandler::start misuse-resistant (hand it the common formats, let the crate compute wFormatNo) #1356 discussion, the straight replacement was preferred over an additive/deprecated path.)Testing
negotiate_formats/audio_format_eq: client-index mapping, the PCM-only/AAC-first regression, empty overlap, and identity equality. (Enabled the lib test harness inCargo.toml.)cargo clippy -p ironrdp-rdpsnd --all-targets -- -D warningsand the example (--features cliprdr,connector,rdpsnd,server) are clean;cargo test -p ironrdp-rdpsndpasses.