diff --git a/Cargo.lock b/Cargo.lock index 6f52aa85b..3f9cfce08 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2852,6 +2852,7 @@ dependencies = [ "ironrdp-pdu", "ironrdp-svc", "tracing", + "visibility", ] [[package]] diff --git a/crates/ironrdp-rdpsnd/Cargo.toml b/crates/ironrdp-rdpsnd/Cargo.toml index 93cabeb7b..bb5aa5937 100644 --- a/crates/ironrdp-rdpsnd/Cargo.toml +++ b/crates/ironrdp-rdpsnd/Cargo.toml @@ -19,6 +19,11 @@ 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" @@ -26,6 +31,7 @@ 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 diff --git a/crates/ironrdp-rdpsnd/src/server.rs b/crates/ironrdp-rdpsnd/src/server.rs index dd0026dee..6b72bdb5f 100644 --- a/crates/ironrdp-rdpsnd/src/server.rs +++ b/crates/ironrdp-rdpsnd/src/server.rs @@ -28,34 +28,91 @@ pub enum RdpsndServerMessage { Error(Box), } +/// 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; + /// [`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>; /// 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 { + 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 => { diff --git a/crates/ironrdp-testsuite-core/Cargo.toml b/crates/ironrdp-testsuite-core/Cargo.toml index 969c3b784..689da1ef3 100644 --- a/crates/ironrdp-testsuite-core/Cargo.toml +++ b/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" diff --git a/crates/ironrdp-testsuite-core/tests/rdpsnd/mod.rs b/crates/ironrdp-testsuite-core/tests/rdpsnd/mod.rs index 20c2d1623..64ec4134d 100644 --- a/crates/ironrdp-testsuite-core/tests/rdpsnd/mod.rs +++ b/crates/ironrdp-testsuite-core/tests/rdpsnd/mod.rs @@ -1,4 +1,5 @@ mod client; +mod server; use std::borrow::Cow; diff --git a/crates/ironrdp-testsuite-core/tests/rdpsnd/server.rs b/crates/ironrdp-testsuite-core/tests/rdpsnd/server.rs new file mode 100644 index 000000000..fddd202db --- /dev/null +++ b/crates/ironrdp-testsuite-core/tests/rdpsnd/server.rs @@ -0,0 +1,244 @@ +//! Server-side tests for `ironrdp-rdpsnd`. +//! +//! Two layers: +//! - the crate-private `negotiate_formats` / `audio_format_eq` helpers, exposed +//! to this testsuite via the rdpsnd crate's private `__test` feature (the lib +//! itself has no inline test harness — `test = false`); +//! - the `SvcProcessor` negotiation wiring, driven black-box through the public +//! surface (no `__test` shim needed). + +use std::sync::{Arc, Mutex}; + +use ironrdp_core::encode_vec; +use ironrdp_rdpsnd::pdu::{ + AudioFormat, AudioFormatFlags, ClientAudioFormatPdu, ClientAudioOutputPdu, TrainingConfirmPdu, Version, WaveFormat, +}; +use ironrdp_rdpsnd::server::{ + NegotiatedFormat, RdpsndError, RdpsndServer, RdpsndServerHandler, audio_format_eq, negotiate_formats, +}; +use ironrdp_svc::SvcProcessor as _; + +fn fmt(format: WaveFormat, rate: u32) -> AudioFormat { + AudioFormat { + format, + n_channels: 2, + n_samples_per_sec: rate, + n_avg_bytes_per_sec: rate * 4, + n_block_align: 4, + bits_per_sample: 16, + data: None, + } +} + +// ============================================================================ +// `negotiate_formats` / `audio_format_eq` helpers (via the `__test` feature) +// ============================================================================ + +#[test] +fn wformat_no_addresses_the_client_list_not_the_server_list() { + // Server prefers AAC over PCM; the client lists them in the opposite + // order. wFormatNo must follow the CLIENT's indices. + let server = [fmt(WaveFormat::AAC_MS, 44100), fmt(WaveFormat::PCM, 44100)]; + let client = [fmt(WaveFormat::PCM, 44100), fmt(WaveFormat::AAC_MS, 44100)]; + + let common = negotiate_formats(&server, &client); + + // Ordering follows the server's preference (AAC first)... + assert_eq!(common.len(), 2); + assert_eq!(common[0].format().format, WaveFormat::AAC_MS); + assert_eq!(common[1].format().format, WaveFormat::PCM); + // ...but each wFormatNo is the position in the CLIENT list. + assert_eq!(common[0].wformat_no(), 1); // AAC is client index 1 + assert_eq!(common[1].wformat_no(), 0); // PCM is client index 0 +} + +#[test] +fn pcm_only_client_gets_a_valid_client_index() { + // Regression for the --enable-aac trap: server advertises [AAC, PCM] + // but a PCM-only client must get wFormatNo 0 (its sole index), not + // PCM's server-list index of 1 (which the client would reject). + let server = [fmt(WaveFormat::AAC_MS, 44100), fmt(WaveFormat::PCM, 44100)]; + let client = [fmt(WaveFormat::PCM, 44100)]; + + let common = negotiate_formats(&server, &client); + + assert_eq!(common.len(), 1); + assert_eq!(common[0].format().format, WaveFormat::PCM); + assert_eq!(common[0].wformat_no(), 0); +} + +#[test] +fn no_shared_format_yields_empty() { + let server = [fmt(WaveFormat::OPUS, 48000)]; + let client = [fmt(WaveFormat::PCM, 44100)]; + assert!(negotiate_formats(&server, &client).is_empty()); +} + +#[test] +fn equality_ignores_derived_fields_but_not_extra_data() { + let mut a = fmt(WaveFormat::PCM, 44100); + let mut b = fmt(WaveFormat::PCM, 44100); + + // The two derived fields are computable and a client need not echo them — + // differing there is still the same format. + b.n_avg_bytes_per_sec = 0; + b.n_block_align = 99; + assert!(audio_format_eq(&a, &b)); + + // The codec extra-data blob IS significant (e.g. AAC config): a differing + // `data` is a different format, even with identical WAVEFORMATEX fields. + a.data = Some(vec![1, 2, 3]); + b.data = None; + assert!(!audio_format_eq(&a, &b)); + + // A differing identity field (sample rate) is a different format. + let c = fmt(WaveFormat::PCM, 48000); + assert!(!audio_format_eq(&a, &c)); +} + +#[test] +fn extra_data_must_match_for_otherwise_identical_formats() { + // Two AAC formats identical in every WAVEFORMATEX field but carrying + // different HEAACWAVEINFO extra data are genuinely incompatible and must + // not be treated as a match (the MS-RDPEA 2.2.2.1.1 `data` case). + let mut server = fmt(WaveFormat::AAC_MS, 44100); + server.data = Some(vec![0x11, 0x90]); + let mut client = fmt(WaveFormat::AAC_MS, 44100); + client.data = Some(vec![0x12, 0x08]); + + assert!(negotiate_formats(&[server], &[client]).is_empty()); +} + +// ============================================================================ +// `SvcProcessor` negotiation wiring (black-box, public surface only) +// ============================================================================ + +#[derive(Debug, Default)] +struct Recording { + choose_format_calls: usize, + start_calls: usize, + chosen_wformat: Option, +} + +#[derive(Debug)] +struct FakeHandler { + formats: Vec, + rec: Arc>, + start_ok: bool, +} + +impl RdpsndServerHandler for FakeHandler { + fn get_formats(&self) -> &[AudioFormat] { + &self.formats + } + + fn choose_format<'a>(&mut self, common: &'a [NegotiatedFormat]) -> Option<&'a NegotiatedFormat> { + let mut rec = self.rec.lock().expect("poisoned"); + rec.choose_format_calls += 1; + let chosen = common.first(); + rec.chosen_wformat = chosen.map(NegotiatedFormat::wformat_no); + chosen + } + + fn start(&mut self, _format: &NegotiatedFormat) -> Result<(), Box> { + self.rec.lock().expect("poisoned").start_calls += 1; + if self.start_ok { + Ok(()) + } else { + Err(Box::new(std::io::Error::other("simulated init failure"))) + } + } + + fn stop(&mut self) {} +} + +/// Drive a fresh server through the handshake (server announce → client formats +/// → training confirm) so the negotiation (`choose_format` + `start`) runs. +/// Client version is V5 (< V6) to skip the optional Quality Mode step. +fn drive_to_ready(server: &mut RdpsndServer, client_formats: Vec) { + server.start().expect("server announce"); + + let client_af = ClientAudioOutputPdu::AudioFormat(ClientAudioFormatPdu { + version: Version::V5, + flags: AudioFormatFlags::empty(), + formats: client_formats, + volume_left: 0, + volume_right: 0, + pitch: 0, + dgram_port: 0, + }); + server + .process(&encode_vec(&client_af).expect("encode client formats")) + .expect("process client formats"); + + let confirm = ClientAudioOutputPdu::TrainingConfirm(TrainingConfirmPdu { + timestamp: 0, + pack_size: 0, + }); + server + .process(&encode_vec(&confirm).expect("encode training confirm")) + .expect("process training confirm"); +} + +#[test] +fn processor_skips_choose_format_when_nothing_in_common() { + let rec = Arc::new(Mutex::new(Recording::default())); + let mut server = RdpsndServer::new(Box::new(FakeHandler { + formats: vec![fmt(WaveFormat::PCM, 44100)], + rec: Arc::clone(&rec), + start_ok: true, + })); + + // Server offers only PCM; client offers only AAC → no common format. + drive_to_ready(&mut server, vec![fmt(WaveFormat::AAC_MS, 44100)]); + + { + let rec = rec.lock().expect("poisoned"); + assert_eq!( + rec.choose_format_calls, 0, + "choose_format must be skipped when common is empty" + ); + assert_eq!(rec.start_calls, 0); + } + // Nothing negotiated → no format committed. + assert!(server.wave(vec![0; 4], 0).is_err()); +} + +#[test] +fn processor_calls_start_once_and_streams_on_success() { + let rec = Arc::new(Mutex::new(Recording::default())); + let mut server = RdpsndServer::new(Box::new(FakeHandler { + formats: vec![fmt(WaveFormat::PCM, 44100)], + rec: Arc::clone(&rec), + start_ok: true, + })); + + drive_to_ready(&mut server, vec![fmt(WaveFormat::PCM, 44100)]); + + { + let rec = rec.lock().expect("poisoned"); + assert_eq!(rec.choose_format_calls, 1); + assert_eq!(rec.start_calls, 1, "start must be called exactly once"); + assert_eq!(rec.chosen_wformat, Some(0)); // PCM is the client's only entry + } + // Format committed → waves stream. + assert!(server.wave(vec![0; 4], 0).is_ok()); +} + +#[test] +fn processor_declines_when_start_fails() { + let rec = Arc::new(Mutex::new(Recording::default())); + let mut server = RdpsndServer::new(Box::new(FakeHandler { + formats: vec![fmt(WaveFormat::PCM, 44100)], + rec: Arc::clone(&rec), + start_ok: false, // simulate an encoder/init failure + })); + + drive_to_ready(&mut server, vec![fmt(WaveFormat::PCM, 44100)]); + + assert_eq!(rec.lock().expect("poisoned").start_calls, 1); + // `start` returned Err → the crate rolls `format_no` back to None and + // declines, so no audio is streamed — rather than a silent + // "negotiated, no audio" state with a committed format and no producer. + assert!(server.wave(vec![0; 4], 0).is_err()); +} diff --git a/crates/ironrdp/examples/server.rs b/crates/ironrdp/examples/server.rs index 71db6d021..ef5d44028 100644 --- a/crates/ironrdp/examples/server.rs +++ b/crates/ironrdp/examples/server.rs @@ -5,14 +5,15 @@ use core::net::SocketAddr; use core::num::{NonZeroU16, NonZeroUsize}; +use std::io; use std::path::PathBuf; use std::sync::{Arc, Mutex}; use anyhow::Context as _; use ironrdp::cliprdr::backend::{CliprdrBackend, CliprdrBackendFactory}; use ironrdp::connector::DesktopSize; -use ironrdp::rdpsnd::pdu::{AudioFormat, ClientAudioFormatPdu, WaveFormat}; -use ironrdp::rdpsnd::server::{RdpsndServerHandler, RdpsndServerMessage}; +use ironrdp::rdpsnd::pdu::{AudioFormat, WaveFormat}; +use ironrdp::rdpsnd::server::{NegotiatedFormat, RdpsndError, RdpsndServerHandler, RdpsndServerMessage}; use ironrdp::server::tokio::sync::mpsc::UnboundedSender; use ironrdp::server::tokio::time::{self, Duration, sleep}; use ironrdp::server::{ @@ -255,17 +256,6 @@ struct SndHandler { task: Option>, } -impl SndHandler { - fn choose_format(&self, client_formats: &[AudioFormat]) -> Option { - for (n, fmt) in client_formats.iter().enumerate() { - if self.get_formats().contains(fmt) { - return u16::try_from(n).ok(); - } - } - None - } -} - impl RdpsndServerHandler for SndHandler { fn get_formats(&self) -> &[AudioFormat] { &[ @@ -290,30 +280,32 @@ impl RdpsndServerHandler for SndHandler { ] } - fn start(&mut self, client_format: &ClientAudioFormatPdu) -> Option { - debug!(?client_format); + fn choose_format<'a>(&mut self, common: &'a [NegotiatedFormat]) -> Option<&'a NegotiatedFormat> { + debug!(?common); - let Some(nfmt) = self.choose_format(&client_format.formats) else { - return Some(0); - }; + // The crate hands us the formats common to both peers in our preference + // order; take the most-preferred one. + common.first() + } - let fmt = client_format.formats[usize::from(nfmt)].clone(); + fn start(&mut self, format: &NegotiatedFormat) -> Result<(), Box> { + let fmt = format.format().clone(); let mut opus_enc = if fmt.format == WaveFormat::OPUS { let n_channels: opus2::Channels = match fmt.n_channels { 1 => opus2::Channels::Mono, 2 => opus2::Channels::Stereo, - n => { - warn!("Invalid OPUS channels: {}", n); - return Some(0); - } + // Init failure: decline the format instead of leaving the channel + // negotiated-but-silent (the crate logs the error and skips audio). + n => return Err(Box::new(io::Error::other(format!("invalid OPUS channels: {n}")))), }; match opus2::Encoder::new(fmt.n_samples_per_sec, n_channels, opus2::Application::Audio) { Ok(enc) => Some(enc), Err(err) => { - warn!("Failed to create OPUS encoder: {}", err); - return Some(0); + return Err(Box::new(io::Error::other(format!( + "failed to create OPUS encoder: {err}" + )))); } } } else { @@ -349,7 +341,7 @@ impl RdpsndServerHandler for SndHandler { } })); - Some(nfmt) + Ok(()) } fn stop(&mut self) {