fix(egfx): decode AVC420 sent as an Annex B byte stream

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<T> = Result<T, DecoderError>;
 /// 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<T> = Result<T, DecoderError>;
 /// }
 /// ```
 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<DecodedFrame> {
-            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"))?;
 

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]

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> <NAL> <4-byte BE length> <NAL> ...
@@ -448,8 +484,9 @@ pub fn avc_to_annex_b_into(data: &[u8], out: &mut Vec<u8>) {
 
 /// 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 <NAL> 00 00 00 01 <NAL> ...
@@ -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
 ///

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::<Avc420BitmapStream<'_>>(&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:
 ///

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<u8> {
+/// 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<u8> {
     use openh264::encoder::Encoder;
     use openh264::formats::YUVBuffer;
 
@@ -18,9 +15,16 @@ fn generate_test_avc_bitstream() -> Vec<u8> {
     // 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<u8> {
+    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