feat(rdpsnd)!: misuse-resistant format negotiation for RdpsndServerHandler#1359
Open
clintcan wants to merge 1 commit into
Open
feat(rdpsnd)!: misuse-resistant format negotiation for RdpsndServerHandler#1359clintcan wants to merge 1 commit into
clintcan 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
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.
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.