diff --git a/crates/ironrdp-egfx/src/decode.rs b/crates/ironrdp-egfx/src/decode.rs index d9b1b3164e..411de5a4a6 100644 --- a/crates/ironrdp-egfx/src/decode.rs +++ b/crates/ironrdp-egfx/src/decode.rs @@ -9,11 +9,14 @@ //! # Protocol Context //! //! H.264 data arrives inside [RFX_AVC420_BITMAP_STREAM][1] payloads -//! within `RDPGFX_WIRE_TO_SURFACE_PDU_1` messages. The NAL units -//! are in AVC format (4-byte big-endian length prefix per NAL unit), -//! not Annex B (start code prefix). +//! within `RDPGFX_WIRE_TO_SURFACE_PDU_1` messages. The specification +//! defines it as an Annex B byte stream (start code prefix), which is what +//! servers built on FreeRDP's server library, such as GNOME Remote Desktop, +//! send. Earlier versions of this crate documented AVC format (4-byte +//! big-endian length prefix per NAL unit) instead, so decoders should accept +//! both. //! -//! [1]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/d65c3f9c-2088-4302-90c0-53adc0e11a78 +//! [1]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/5f12c20e-2ea1-4ad1-a2a0-019ee3893731 use core::fmt; @@ -138,8 +141,10 @@ pub type DecoderResult = Result; /// Trait for H.264 (AVC) decoders /// /// Implement this trait to provide H.264 decode capability to the -/// EGFX client. The decoder receives AVC-format NAL units (length-prefixed, -/// not Annex B) from `RFX_AVC420_BITMAP_STREAM` payloads. +/// EGFX client. The decoder receives the H.264 data from +/// `RFX_AVC420_BITMAP_STREAM` payloads: an Annex B byte stream per the +/// specification, or AVC-format NAL units (4-byte BE length prefix) from +/// servers that followed this crate's earlier documentation. /// /// # Thread Safety /// @@ -160,8 +165,8 @@ pub type DecoderResult = Result; /// } /// ``` pub trait H264Decoder: Send { - /// Decode AVC-format H.264 NAL units (4-byte BE length prefix, not Annex B) - /// into an RGBA bitmap. + /// Decode H.264 NAL units, as an Annex B byte stream or in AVC format + /// (4-byte BE length prefix), into an RGBA bitmap. /// /// Frame dimensions may exceed the destination rectangle due to /// macroblock alignment (16x16). The caller crops to fit. @@ -190,9 +195,9 @@ mod openh264_impl { /// H.264 decoder backed by Cisco's OpenH264 library /// - /// This decoder converts AVC-format NAL units to Annex B format - /// (as required by OpenH264), decodes to YUV420p, then converts - /// to RGBA for the client pipeline. + /// This decoder passes Annex B input to OpenH264 as-is and converts + /// AVC-format NAL units to Annex B first (OpenH264 only reads Annex B), + /// decodes to YUV420p, then converts to RGBA for the client pipeline. /// /// # Feature Gates /// @@ -246,11 +251,16 @@ mod openh264_impl { impl H264Decoder for OpenH264Decoder { fn decode(&mut self, data: &[u8]) -> DecoderResult { - crate::pdu::avc_to_annex_b_into(data, &mut self.annex_b_buffer); + let annex_b = if crate::pdu::is_avc_format(data) { + crate::pdu::avc_to_annex_b_into(data, &mut self.annex_b_buffer); + &self.annex_b_buffer + } else { + data + }; let yuv = self .decoder - .decode(&self.annex_b_buffer) + .decode(annex_b) .map_err(|e| DecoderError::new("OpenH264 decode failed", e))? .ok_or_else(|| DecoderError::msg("OpenH264 returned no picture"))?; diff --git a/crates/ironrdp-egfx/src/encode.rs b/crates/ironrdp-egfx/src/encode.rs index ede4e75f85..ae72c01c96 100644 --- a/crates/ironrdp-egfx/src/encode.rs +++ b/crates/ironrdp-egfx/src/encode.rs @@ -257,10 +257,10 @@ mod tests { use super::*; use crate::decode::H264Decoder as _; - /// Round-trip through the reference encoder and the reference decoder, - /// crossing the documented format boundary explicitly: the encoder - /// produces Annex B (the wire format), the decoder consumes AVC - /// length-prefixed input, and `pdu::annex_b_to_avc` is the bridge. + /// Round-trip through the reference encoder and the reference decoder. + /// The encoder produces Annex B (the wire format). The decoder accepts + /// both Annex B and AVC length-prefixed input; this test converts with + /// `pdu::annex_b_to_avc` to exercise the AVC path. /// /// Run with: `cargo test -p ironrdp-egfx --features openh264-bundled` #[test] diff --git a/crates/ironrdp-egfx/src/pdu/avc.rs b/crates/ironrdp-egfx/src/pdu/avc.rs index 67088c2d04..46476c393a 100644 --- a/crates/ironrdp-egfx/src/pdu/avc.rs +++ b/crates/ironrdp-egfx/src/pdu/avc.rs @@ -366,12 +366,48 @@ impl Avc420Region { } } +/// Whether `data` is H.264 AVC format (4-byte big-endian length prefixes) +/// rather than an Annex B byte stream +/// +/// MS-RDPEGFX specifies Annex B, so a buffer that starts with a start code +/// (`00 00 01` or `00 00 00 01`) is Annex B. Any other buffer is AVC format +/// if its length prefixes chain exactly to the end of the buffer. +/// +/// Checking the chain first would misread valid Annex B: `00 00 01 67` +/// followed by 359 bytes is also a well-formed one-unit AVC buffer. Checking +/// the start code first moves the ambiguity to AVC senders whose first NAL +/// unit is 1 or 256..=511 bytes long, which don't follow the specification. +pub fn is_avc_format(data: &[u8]) -> bool { + if data.starts_with(&[0x00, 0x00, 0x01]) || data.starts_with(&[0x00, 0x00, 0x00, 0x01]) { + return false; + } + let mut offset = 0usize; + loop { + if offset == data.len() { + return !data.is_empty(); + } + let Some(len_bytes) = data.get(offset..offset + 4).and_then(|b| <[u8; 4]>::try_from(b).ok()) else { + return false; + }; + let next = usize::try_from(u32::from_be_bytes(len_bytes)) + .ok() + .filter(|&len| len > 0) + .and_then(|len| (offset + 4).checked_add(len)); + match next { + Some(next) if next <= data.len() => offset = next, + _ => return false, + } + } +} + /// Convert H.264 AVC format to Annex B format /// -/// OpenH264 and similar decoders consume Annex B format (start code prefixed), -/// but MS-RDPEGFX delivers AVC format (length-prefixed NAL units). This helper -/// converts between the two by replacing each 4-byte big-endian length prefix -/// with the 4-byte Annex B start code `[0x00, 0x00, 0x00, 0x01]`. +/// OpenH264 and similar decoders consume Annex B format (start code prefixed). +/// MS-RDPEGFX specifies Annex B on the wire, but earlier versions of this +/// crate documented AVC format (length-prefixed NAL units), so senders may +/// still use it. This helper converts AVC input by +/// replacing each 4-byte big-endian length prefix with the 4-byte Annex B +/// start code `[0x00, 0x00, 0x00, 0x01]`. /// /// ```text /// AVC: <4-byte BE length> <4-byte BE length> ... @@ -448,8 +484,9 @@ pub fn avc_to_annex_b_into(data: &[u8], out: &mut Vec) { /// Convert H.264 Annex B format to AVC format /// -/// MS-RDPEGFX requires AVC format (length-prefixed NAL units), -/// but most encoders output Annex B format (start code prefixed). +/// MS-RDPEGFX specifies Annex B (start code prefixed) on the wire, so this is +/// only needed to produce AVC format (length-prefixed NAL units) for +/// consumers that require it. /// /// ```text /// Annex B: 00 00 00 01 00 00 00 01 ... @@ -557,7 +594,8 @@ pub const fn align_to_16(dimension: u32) -> u32 { /// # Arguments /// /// * `regions` - List of regions with their encoding parameters -/// * `h264_data` - H.264 encoded data (should be in AVC format, not Annex B) +/// * `h264_data` - H.264 encoded data as an Annex B byte stream, which is the +/// format MS-RDPEGFX specifies for `RFX_AVC420_BITMAP_STREAM` /// /// # Returns /// diff --git a/crates/ironrdp-fuzzing/src/oracles/mod.rs b/crates/ironrdp-fuzzing/src/oracles/mod.rs index 9de240563a..2e713e1874 100644 --- a/crates/ironrdp-fuzzing/src/oracles/mod.rs +++ b/crates/ironrdp-fuzzing/src/oracles/mod.rs @@ -395,18 +395,21 @@ pub fn egfx_round_trip(data: &[u8]) { /// AVC420 decode-side wrapper fuzz oracle. /// /// Fuzzes the IronRDP wrapper layer between a wire `Avc420BitmapStream` and -/// the consumer's `H264Decoder`. Specifically targets `avc_to_annex_b`, the -/// AVC-length-prefix to Annex-B conversion that runs before OpenH264 sees -/// any bytes. +/// the consumer's `H264Decoder`: `is_avc_format`, which decides whether the +/// input is length-prefixed, and `avc_to_annex_b`, the conversion that +/// `OpenH264Decoder` runs on length-prefixed input before OpenH264 sees it +/// (Annex B input is passed through unchanged). /// /// The oracle runs two paths on each input: /// -/// - Direct: call `avc_to_annex_b(data)` on the raw fuzz input. This -/// exercises the wrapper on arbitrary byte distributions, including +/// - Direct: run `is_avc_format` and `avc_to_annex_b` on the raw fuzz input, +/// the conversion unconditionally so malformed length prefixes reach it. +/// This exercises the wrapper on arbitrary byte distributions, including /// inputs that do not parse as `Avc420BitmapStream`. -/// - Decode-chain: try `Avc420BitmapStream::decode(data)`; on success, call -/// `avc_to_annex_b(stream.data)`. This exercises the wrapper on the -/// realistic post-decode payload distribution. +/// - Decode-chain: try `Avc420BitmapStream::decode(data)`; on success, make +/// the same dispatch as `OpenH264Decoder::decode` on the stream's payload. +/// This exercises the wrapper on the realistic post-decode payload +/// distribution. /// /// What this catches: panics in the wrapper, OOM via attacker-controlled /// NAL length encoding, contract violations on the produced Annex-B byte @@ -416,13 +419,16 @@ pub fn egfx_round_trip(data: &[u8]) { /// post-OpenH264 YUV-to-RGBA conversion path in `OpenH264Decoder::decode`, /// AVC444 luma plus chroma split (covered by a sibling target). pub fn egfx_avc420_decode(data: &[u8]) { - use ironrdp_egfx::pdu::{Avc420BitmapStream, avc_to_annex_b}; + use ironrdp_egfx::pdu::{Avc420BitmapStream, avc_to_annex_b, is_avc_format}; + let _ = is_avc_format(data); let _ = avc_to_annex_b(data); let mut cursor = ironrdp_core::ReadCursor::new(data); if let Ok(stream) = ironrdp_core::decode_cursor::>(&mut cursor) { - let _ = avc_to_annex_b(stream.data); + if is_avc_format(stream.data) { + let _ = avc_to_annex_b(stream.data); + } } } @@ -1297,9 +1303,10 @@ pub fn message_decoding_invariants(data: &[u8]) { /// /// Sibling of [`egfx_avc420_decode`]. Fuzzes the IronRDP wrapper layer between /// a wire `Avc444BitmapStream` and the consumer's `H264Decoder`. Targets the -/// AVC-length-prefix to Annex-B conversion that runs on each of the two +/// AVC-length-prefix to Annex-B conversion, called unconditionally (without +/// the `is_avc_format` dispatch) on the raw input and on each of the two /// underlying `Avc420BitmapStream`s (luma plus optional chroma) per -/// MS-RDPEGFX 2.2.4.4. +/// MS-RDPEGFX 2.2.4.4, so malformed length prefixes reach it. /// /// The oracle runs three paths on each input: /// diff --git a/crates/ironrdp-testsuite-core/tests/egfx/decode.rs b/crates/ironrdp-testsuite-core/tests/egfx/decode.rs index a12821a73b..d85231b572 100644 --- a/crates/ironrdp-testsuite-core/tests/egfx/decode.rs +++ b/crates/ironrdp-testsuite-core/tests/egfx/decode.rs @@ -4,12 +4,9 @@ use ironrdp_egfx::decode::{H264Decoder, OpenH264Decoder}; // Test Helpers // ============================================================================ -/// Generate a minimal AVC-format H.264 bitstream by encoding a black 16x16 frame -/// -/// The encoder produces Annex B format (start code prefixed). This function -/// converts the output to AVC format (4-byte BE length prefixed) to exercise -/// the full decode pipeline including AVC-to-Annex-B conversion. -fn generate_test_avc_bitstream() -> Vec { +/// Generate a minimal Annex B H.264 bitstream (start code prefixed) by +/// encoding a black 16x16 frame. This is the format MS-RDPEGFX specifies. +fn generate_test_annex_b_bitstream() -> Vec { use openh264::encoder::Encoder; use openh264::formats::YUVBuffer; @@ -18,9 +15,16 @@ fn generate_test_avc_bitstream() -> Vec { // Black 16x16 YUV420p frame (all zeros) let yuv = YUVBuffer::new(16, 16); let bitstream = encoder.encode(&yuv).expect("encode should succeed"); - let annex_b = bitstream.to_vec(); + bitstream.to_vec() +} - annex_b_to_avc(&annex_b) +/// Generate a minimal AVC-format H.264 bitstream by encoding a black 16x16 frame +/// +/// The encoder produces Annex B format (start code prefixed). This function +/// converts the output to AVC format (4-byte BE length prefixed) to exercise +/// the full decode pipeline including AVC-to-Annex-B conversion. +fn generate_test_avc_bitstream() -> Vec { + annex_b_to_avc(&generate_test_annex_b_bitstream()) } /// Convert Annex B format NAL units to AVC format (4-byte BE length prefix) @@ -120,6 +124,96 @@ fn test_openh264_decoder_reset() { assert_eq!(frame.height(), 16); } +// ============================================================================ +// Annex B Input Tests +// ============================================================================ + +#[test] +fn test_openh264_decode_annex_b() { + // MS-RDPEGFX specifies an Annex B byte stream; it must decode as sent. + let annex_b = generate_test_annex_b_bitstream(); + assert!(annex_b.starts_with(&[0x00, 0x00, 0x00, 0x01])); + + let mut decoder = OpenH264Decoder::new().expect("decoder should initialize"); + let frame = decoder.decode(&annex_b).expect("Annex B decode should succeed"); + assert_eq!(frame.width(), 16); + assert_eq!(frame.height(), 16); +} + +#[test] +fn test_openh264_decode_annex_b_with_access_unit_delimiter() { + // GNOME Remote Desktop starts each frame with an access unit delimiter + // (NAL type 9). Read as AVC format, `00 00 00 01` becomes a 1-byte NAL + // and the bytes after it a bogus length, so the frame never decodes. + let mut annex_b = vec![0x00, 0x00, 0x00, 0x01, 0x09, 0x10]; + annex_b.extend_from_slice(&generate_test_annex_b_bitstream()); + + let mut decoder = OpenH264Decoder::new().expect("decoder should initialize"); + let frame = decoder + .decode(&annex_b) + .expect("Annex B decode with an access unit delimiter should succeed"); + assert_eq!(frame.width(), 16); + assert_eq!(frame.height(), 16); +} + +#[test] +fn test_openh264_decode_annex_b_three_byte_start_codes() { + // Annex B also allows 3-byte start codes (00 00 01). + let annex_b = generate_test_annex_b_bitstream(); + let mut short_codes = Vec::with_capacity(annex_b.len()); + let mut i = 0; + while i < annex_b.len() { + if annex_b[i..].starts_with(&[0x00, 0x00, 0x00, 0x01]) { + short_codes.extend_from_slice(&[0x00, 0x00, 0x01]); + i += 4; + } else { + short_codes.push(annex_b[i]); + i += 1; + } + } + assert!(short_codes.starts_with(&[0x00, 0x00, 0x01])); + + let mut decoder = OpenH264Decoder::new().expect("decoder should initialize"); + let frame = decoder + .decode(&short_codes) + .expect("Annex B decode with 3-byte start codes should succeed"); + assert_eq!(frame.width(), 16); + assert_eq!(frame.height(), 16); +} + +#[test] +fn test_openh264_decode_annex_b_that_also_parses_as_avc() { + // `00 00 01 67` read as a length is 0x167 = 359, so a 363-byte Annex B + // buffer that starts with a 3-byte start code and an SPS is also a + // well-formed one-unit AVC buffer. The start code must win. Trailing + // zero bytes are allowed after the last NAL unit in Annex B. + let mut annex_b = vec![0x00, 0x00, 0x01]; + annex_b.extend_from_slice(&generate_test_annex_b_bitstream()[4..]); + assert!(annex_b.starts_with(&[0x00, 0x00, 0x01, 0x67])); + assert!(annex_b.len() <= 363, "test frame too large: {}", annex_b.len()); + annex_b.resize(363, 0); + + let mut decoder = OpenH264Decoder::new().expect("decoder should initialize"); + let frame = decoder + .decode(&annex_b) + .expect("Annex B that also parses as AVC should decode as Annex B"); + assert_eq!(frame.width(), 16); + assert_eq!(frame.height(), 16); + assert!(!ironrdp_egfx::pdu::is_avc_format(&annex_b)); +} + +#[test] +fn test_is_avc_format() { + use ironrdp_egfx::pdu::is_avc_format; + + let annex_b = generate_test_annex_b_bitstream(); + assert!(!is_avc_format(&annex_b)); + assert!(is_avc_format(&annex_b_to_avc(&annex_b))); + assert!(!is_avc_format(&[])); + // A length that runs past the end is not AVC format. + assert!(!is_avc_format(&[0x00, 0x00, 0x00, 0x09, 0x67])); +} + // ============================================================================ // Error Path Tests // ============================================================================ @@ -128,8 +222,7 @@ fn test_openh264_decoder_reset() { fn test_decode_empty_input() { let mut decoder = OpenH264Decoder::new().expect("decoder should initialize"); - // Empty input has no NAL units -- the AVC-to-Annex-B converter - // produces nothing, and OpenH264 returns no picture. + // Empty input has no NAL units, so OpenH264 returns no picture. let result = decoder.decode(&[]); assert!(result.is_err(), "decoding empty input should fail"); } @@ -138,8 +231,8 @@ fn test_decode_empty_input() { fn test_decode_truncated_nal_length() { let mut decoder = OpenH264Decoder::new().expect("decoder should initialize"); - // Less than 4 bytes: can't even read the NAL length prefix. - // The converter produces an empty Annex B buffer. + // Less than 4 bytes: can't even read a NAL length prefix, so the input + // is passed to OpenH264 as Annex B, which finds no picture in it. let result = decoder.decode(&[0x00, 0x00]); assert!(result.is_err(), "truncated NAL length should fail"); } @@ -148,8 +241,9 @@ fn test_decode_truncated_nal_length() { fn test_decode_nal_length_exceeds_buffer() { let mut decoder = OpenH264Decoder::new().expect("decoder should initialize"); - // NAL length says 100 bytes but only 2 bytes follow. - // The converter discards the malformed NAL and produces empty output. + // NAL length says 100 bytes but only 2 bytes follow, so the input is not + // valid AVC framing. It is passed to OpenH264 as Annex B, which finds no + // picture in it. let mut data = Vec::new(); data.extend_from_slice(&100u32.to_be_bytes()); data.extend_from_slice(&[0x67, 0x00]); // Partial NAL