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

crates/ironrdp-rdpsnd/Cargo.toml

@@ -19,13 +19,19 @@ test = false
 [features]
 default = []
 std = []
+# Internal (PRIVATE!) feature used to aid testing.
+# Don't rely on this whatsoever. It may disappear at any time.
+# It uses `visibility` to expose otherwise-private negotiation helpers to the
+# integration testsuite (the lib has no inline test harness — `test = false`).
+__test = ["dep:visibility"]
 
 [dependencies]
 bitflags = "2.11"
 tracing = { version = "0.1", features = ["log"] }
 ironrdp-svc = { path = "../ironrdp-svc", version = "0.7" } # public
 ironrdp-core = { path = "../ironrdp-core", version = "0.2", features = ["alloc"] }
 ironrdp-pdu = { path = "../ironrdp-pdu", version = "0.8", features = ["alloc"] } # public
+visibility = { version = "0.1", optional = true }
 
 [lints]
 workspace = true

crates/ironrdp-rdpsnd/src/server.rs

@@ -28,34 +28,91 @@ pub enum RdpsndServerMessage {
     Error(Box<dyn RdpsndError>),
 }
 
+/// A server-offered audio format that the client also advertised support for,
+/// paired with the `wFormatNo` the client expects for it on the wire.
+///
+/// The crate computes the set of these — the intersection of the server's
+/// [`get_formats`] and the client's accepted formats — and hands it to
+/// [`RdpsndServerHandler::choose_format`], which returns the one to stream.
+///
+/// `wformat_no` is intentionally private and there is no public constructor:
+/// a handler can neither build nor mutate a `NegotiatedFormat`, so the index
+/// stamped onto every Wave/Wave2 PDU is always a valid position in the
+/// client's own format list. This makes it impossible to emit an out-of-range
+/// `wFormatNo` (which a compliant client rejects, silently dropping all audio
+/// — the classic footgun of the old index-returning API).
+///
+/// [`get_formats`]: RdpsndServerHandler::get_formats
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct NegotiatedFormat {
+    /// The negotiated audio format (common to server and client).
+    format: pdu::AudioFormat,
+    /// Position of `format` in the client's Client Audio Formats list — the
+    /// `wFormatNo` the client resolves each wave against. Crate-owned.
+    wformat_no: u16,
+}
+
+impl NegotiatedFormat {
+    /// The negotiated audio format — common to both server and client, and the
+    /// one the returned wave data should match.
+    pub fn format(&self) -> &pdu::AudioFormat {
+        &self.format
+    }
+
+    /// Test-only accessor for the crate-private `wformat_no`, exposed for the
+    /// integration testsuite behind the private `__test` feature. Not a stable API.
+    #[cfg(feature = "__test")]
+    #[doc(hidden)]
+    pub fn wformat_no(&self) -> u16 {
+        self.wformat_no
+    }
+}
+
 /// Handler for the server side of the Audio Output Virtual Channel (`RDPSND`).
 ///
-/// Implementations supply the list of audio formats the server offers, decide
-/// which format to use once the client replies, and produce the audio waves to
-/// stream (via [`RdpsndServer::wave`]).
+/// Implementations supply the list of audio formats the server offers, choose
+/// which negotiated format to use once the client replies, and produce the
+/// audio waves to stream (via [`RdpsndServer::wave`]).
 pub trait RdpsndServerHandler: Send + core::fmt::Debug {
     /// The audio formats the server advertises in the Server Audio Formats and
     /// Version PDU (MS-RDPEA 2.2.2.1).
     fn get_formats(&self) -> &[pdu::AudioFormat];
 
-    /// Called once the client has replied with the formats it accepts
-    /// (`client_format`, the Client Audio Formats and Version PDU). Returns the
-    /// `wFormatNo` to stamp on every subsequent Wave/Wave2 PDU, or [`None`] if
-    /// no offered format is acceptable (no audio is then streamed).
+    /// Select which format to stream, once the client has replied with the
+    /// formats it accepts.
+    ///
+    /// `common` is the set of formats from [`get_formats`] that the client also
+    /// advertised, in the server's preference order; each carries the
+    /// `wFormatNo` the client expects, so the crate — not the handler — owns
+    /// the index arithmetic and the MS-RDPEA rule that `wFormatNo` addresses
+    /// the *client's* list. `common` is never empty: when server and client
+    /// share no format, this method is not called and no audio is streamed.
     ///
-    /// **The returned index addresses `client_format.formats` — the formats the
-    /// client just echoed back — NOT the server's own [`get_formats`] list.**
-    /// The client resolves each wave's format as `ClientFormats[wFormatNo]`
-    /// against the list *it* sent, and a compliant client rejects any
-    /// `wFormatNo >= client_format.formats.len()`, silently dropping all audio.
-    /// The client's list is its accepted subset of the server's formats, so the
-    /// two lists generally differ in both length and ordering; an index into
-    /// [`get_formats`] only happens to work when the chosen format sits at the
-    /// same position in both. Pick the format you intend to send, then return
-    /// its position within `client_format.formats`.
+    /// Return the [`NegotiatedFormat`] to stream (a reference borrowed from
+    /// `common`), or [`None`] to decline. Returning a borrow from `common`
+    /// — rather than an index or a constructed value — makes it impossible to
+    /// pick a format the client did not accept or to produce an invalid
+    /// `wFormatNo`. This is a pure selection step: any encoder/producer setup
+    /// belongs in [`start`], which the crate calls next with the chosen format.
     ///
     /// [`get_formats`]: RdpsndServerHandler::get_formats
-    fn start(&mut self, client_format: &ClientAudioFormatPdu) -> Option<u16>;
+    /// [`start`]: RdpsndServerHandler::start
+    fn choose_format<'a>(&mut self, common: &'a [NegotiatedFormat]) -> Option<&'a NegotiatedFormat>;
+
+    /// Begin streaming with the `format` just selected by [`choose_format`].
+    ///
+    /// Called once per session, immediately after a successful
+    /// [`choose_format`]. This is the lifecycle hook: initialize encoder state,
+    /// spawn the producer, etc. Waves are then emitted via [`RdpsndServer::wave`].
+    ///
+    /// Return `Err` if initialization fails (e.g. the encoder can't be created).
+    /// The crate then **declines the negotiated format** — exactly as if
+    /// [`choose_format`] had returned [`None`] — rather than leaving the channel
+    /// "negotiated" but silently producing no audio. The error is logged by the
+    /// crate.
+    ///
+    /// [`choose_format`]: RdpsndServerHandler::choose_format
+    fn start(&mut self, format: &NegotiatedFormat) -> Result<(), Box<dyn RdpsndError>>;
 
     /// Called when the audio stream is torn down (e.g. the client closed the
     /// channel or the session ended).
@@ -173,6 +230,52 @@ impl RdpsndServer {
     }
 }
 
+/// Build the set of formats common to the server (`server_formats`, kept in the
+/// server's preference order) and the client (`client_formats`), each tagged
+/// with its `wFormatNo` — its index in the *client's* list, which is what the
+/// client resolves waves against (MS-RDPEA). The result mirrors the server's
+/// ordering so the handler can express preference simply by `get_formats`
+/// order, while the `wFormatNo` always points into the client list.
+#[cfg_attr(feature = "__test", visibility::make(pub))]
+fn negotiate_formats(
+    server_formats: &[pdu::AudioFormat],
+    client_formats: &[pdu::AudioFormat],
+) -> Vec<NegotiatedFormat> {
+    server_formats
+        .iter()
+        .filter_map(|server_format| {
+            client_formats
+                .iter()
+                .position(|client_fmt| audio_format_eq(client_fmt, server_format))
+                .and_then(|idx| u16::try_from(idx).ok())
+                .map(|wformat_no| NegotiatedFormat {
+                    format: server_format.clone(),
+                    wformat_no,
+                })
+        })
+        .collect()
+}
+
+/// Compare two audio formats for negotiation. The WAVEFORMATEX identity fields
+/// — wave format tag, channel count, sample rate, bit depth — must match, and so
+/// must the codec-specific extra-data blob (`data`).
+///
+/// The two derived fields (`n_avg_bytes_per_sec`, `n_block_align`) are
+/// deliberately ignored: they are computable from the others and a client may
+/// legitimately not echo them back byte-for-byte. The `data` blob is a different
+/// category, though — for codecs whose extra-format bytes carry real
+/// configuration (AAC's HEAACWAVEINFO extra data is the clear case, MS-RDPEA
+/// 2.2.2.1.1's `cbSize` + extra data), ignoring it could match two genuinely
+/// incompatible formats, so it IS compared.
+#[cfg_attr(feature = "__test", visibility::make(pub))]
+fn audio_format_eq(a: &pdu::AudioFormat, b: &pdu::AudioFormat) -> bool {
+    a.format == b.format
+        && a.n_channels == b.n_channels
+        && a.n_samples_per_sec == b.n_samples_per_sec
+        && a.bits_per_sample == b.bits_per_sample
+        && a.data == b.data
+}
+
 impl_as_any!(RdpsndServer);
 
 impl SvcProcessor for RdpsndServer {
@@ -220,8 +323,35 @@ impl SvcProcessor for RdpsndServer {
                     return Ok(vec![]);
                 };
                 let client_format = self.client_format.as_ref().expect("available in this state");
+                // Formats common to server and client, in the server's
+                // preference order, each tagged with its wFormatNo (its
+                // position in the *client's* list). Keeping this in the crate
+                // means the handler never does index arithmetic and can't emit
+                // an out-of-range wFormatNo.
+                let common = negotiate_formats(self.handler.get_formats(), &client_format.formats);
                 self.state = RdpsndState::Ready;
-                self.format_no = self.handler.start(client_format);
+                if common.is_empty() {
+                    debug!("No audio format in common with the client; audio disabled");
+                } else if let Some(chosen) = self.handler.choose_format(&common) {
+                    // `chosen` borrows `common` (a local), not `self`, so the
+                    // handler is free to borrow `&mut self` again for `start`.
+                    let wformat_no = chosen.wformat_no;
+                    // Commit the index BEFORE the `start` lifecycle hook: if `start`
+                    // spawns a producer that emits a wave immediately, `wave()` must
+                    // already see a valid `format_no` rather than racing an unset one.
+                    self.format_no = Some(wformat_no);
+                    if let Err(e) = self.handler.start(chosen) {
+                        // Initialization failed (e.g. the encoder couldn't be
+                        // created). Roll back to a cleanly *declined* state — the
+                        // same outcome as `choose_format` returning `None` — instead
+                        // of leaving the channel "negotiated" but silently producing
+                        // no audio.
+                        error!(error = %e, "rdpsnd handler failed to start; declining the negotiated format");
+                        self.format_no = None;
+                    }
+                } else {
+                    debug!("Handler declined every common audio format; audio disabled");
+                }
                 vec![]
             }
             RdpsndState::Ready => {

crates/ironrdp-testsuite-core/Cargo.toml

@@ -52,7 +52,7 @@ ironrdp-input.path = "../ironrdp-input"
 ironrdp-rdcleanpath.path = "../ironrdp-rdcleanpath"
 ironrdp-rdpdr.path = "../ironrdp-rdpdr"
 ironrdp-rdpeusb.path = "../ironrdp-rdpeusb"
-ironrdp-rdpsnd.path = "../ironrdp-rdpsnd"
+ironrdp-rdpsnd = { path = "../ironrdp-rdpsnd", features = ["__test"] }
 ironrdp-server.path = "../ironrdp-server"
 ironrdp-session = { path = "../ironrdp-session", features = ["qoi"] }
 ironrdp-cfg.path = "../ironrdp-cfg"

crates/ironrdp-testsuite-core/tests/rdpsnd/mod.rs

@@ -1,4 +1,5 @@
 mod client;
+mod server;
 
 use std::borrow::Cow;