diff --git a/deku-derive/src/macros/deku_write.rs b/deku-derive/src/macros/deku_write.rs index 461c5e8d1..52dab44b1 100644 --- a/deku-derive/src/macros/deku_write.rs +++ b/deku-derive/src/macros/deku_write.rs @@ -616,7 +616,9 @@ fn emit_field_write( // TODO: This should error on some attributes that don't make sense aren't used? // Such as magic, seek* let crate_ = super::get_crate_name(); - let field_endian = input.id_endian.as_ref(); + // Mirror gen_id_args' read-side precedence (id_endian -> enum endian) so a + // hardcoded `endian="big"` or a bound `endian="endian"` reaches the write. + let field_endian = input.id_endian.as_ref().or(input.endian.as_ref()); #[cfg(feature = "bits")] let field_bits = input.bits.as_ref(); #[cfg(not(feature = "bits"))] diff --git a/src/impls/primitive.rs b/src/impls/primitive.rs index 8ab9e291d..28da99294 100644 --- a/src/impls/primitive.rs +++ b/src/impls/primitive.rs @@ -653,7 +653,7 @@ macro_rules! ImplDekuReadSignExtend { let ret = reader.read_bytes(size.0, &mut buf, order)?; let a = match ret { ReaderRet::Bytes => { - if endian.is_le() { + let value = if endian.is_le() { <$typ>::from_le_bytes(buf.try_into().unwrap()) } else { if size.0 != core::mem::size_of::<$typ>() { @@ -662,6 +662,17 @@ macro_rules! ImplDekuReadSignExtend { buf[..padding].fill(0x00); } <$typ>::from_be_bytes(buf.try_into().unwrap()) + }; + // Sign-extend: a sub-width signed read zero-pads above, so the + // sign bit of the N-byte value must be propagated to the full type. + // A zero-byte read has no sign bit to propagate, and shifting by + // the full type width would overflow, so it is left as-is. + const MAX_TYPE_BITS: usize = BitSize::of::<$typ>().0; + if size.0 == 0 { + value + } else { + let shift = MAX_TYPE_BITS - (size.0 * 8); + (value << shift) >> shift } } #[cfg(all(feature = "bits", feature = "alloc"))] @@ -1712,4 +1723,82 @@ mod tests { TestSignExtendingPanic!(test_sign_extend_i64_panic, i64, 64); #[cfg(feature = "bits")] TestSignExtendingPanic!(test_sign_extend_i128_panic, i128, 128); + + // Regression: sub-width byte-aligned signed reads must sign-extend (ByteSize path). + #[cfg(feature = "bits")] + #[test] + fn test_sign_extend_bytesize_i32_negative() { + // 3-byte LE encoding of -100 + let mut cursor = std::io::Cursor::new([0x9C_u8, 0xFF, 0xFF]); + let mut reader = Reader::new(&mut cursor); + assert_eq!( + -100_i32, + i32::from_reader_with_ctx(&mut reader, (Endian::Little, ByteSize(3))).unwrap() + ); + + // 3-byte BE encoding of -100 + let mut cursor = std::io::Cursor::new([0xFF_u8, 0xFF, 0x9C]); + let mut reader = Reader::new(&mut cursor); + assert_eq!( + -100_i32, + i32::from_reader_with_ctx(&mut reader, (Endian::Big, ByteSize(3))).unwrap() + ); + } + + #[cfg(feature = "bits")] + #[test] + fn test_sign_extend_bytesize_i32_min() { + // 3-byte LE encoding of -8388608 (0x800000 sign-extended) + let mut cursor = std::io::Cursor::new([0x00_u8, 0x00, 0x80]); + let mut reader = Reader::new(&mut cursor); + assert_eq!( + -8388608_i32, + i32::from_reader_with_ctx(&mut reader, (Endian::Little, ByteSize(3))).unwrap() + ); + + // 3-byte BE encoding of -8388608 + let mut cursor = std::io::Cursor::new([0x80_u8, 0x00, 0x00]); + let mut reader = Reader::new(&mut cursor); + assert_eq!( + -8388608_i32, + i32::from_reader_with_ctx(&mut reader, (Endian::Big, ByteSize(3))).unwrap() + ); + } + + // A runtime-computed `bytes` of 0 must keep reading nothing and yield 0, + // not overflow the sign-extension shift. + #[test] + fn test_sign_extend_bytesize_zero() { + let mut cursor = std::io::Cursor::new([0x01_u8, 0x02]); + let mut reader = Reader::new(&mut cursor); + assert_eq!( + 0_i32, + i32::from_reader_with_ctx(&mut reader, (Endian::Little, ByteSize(0))).unwrap() + ); + + let mut cursor = std::io::Cursor::new([0x01_u8, 0x02]); + let mut reader = Reader::new(&mut cursor); + assert_eq!( + 0_i32, + i32::from_reader_with_ctx(&mut reader, (Endian::Big, ByteSize(0))).unwrap() + ); + } + + // Full-width reads take shift == 0 and must round-trip unchanged. + #[test] + fn test_sign_extend_bytesize_full_width() { + let mut cursor = std::io::Cursor::new([0x9C_u8, 0xFF, 0xFF, 0xFF]); + let mut reader = Reader::new(&mut cursor); + assert_eq!( + -100_i32, + i32::from_reader_with_ctx(&mut reader, (Endian::Little, ByteSize(4))).unwrap() + ); + + let mut cursor = std::io::Cursor::new([0xFF_u8, 0xFF, 0xFF, 0x9C]); + let mut reader = Reader::new(&mut cursor); + assert_eq!( + -100_i32, + i32::from_reader_with_ctx(&mut reader, (Endian::Big, ByteSize(4))).unwrap() + ); + } } diff --git a/tests/test_enum.rs b/tests/test_enum.rs index 407a12940..d88d4e5ef 100644 --- a/tests/test_enum.rs +++ b/tests/test_enum.rs @@ -440,3 +440,83 @@ fn test_repr_assignment_with_id_via_ctx() { Message::from_reader((&mut cursor, 0)).unwrap().1 ); } + +/// Regression: the id stored in an id_pat variant must be written with the enum's declared +/// endian. Before the fix it was always written little-endian regardless of endian="big". +#[test] +fn test_id_pat_big_endian_write() { + // The id_pat variant is what triggers the previously-buggy write path. + // A fixed-id variant (like Known) uses a different path that was already correct. + #[derive(PartialEq, Debug, DekuRead, DekuWrite)] + #[deku(id_type = "u16", endian = "big")] + enum E { + #[deku(id = "65535")] + Known, + #[deku(id_pat = "0..=65534")] + Other(u16), + } + + // id_pat variant: 0x0102 big-endian → [0x01, 0x02]; was [0x02, 0x01] before the fix + assert_eq!(E::Other(0x0102).to_bytes().unwrap(), [0x01, 0x02]); + + // round-trip + let mut cursor = Cursor::new([0x01_u8, 0x02]); + let (_, decoded) = E::from_reader((&mut cursor, 0)).unwrap(); + assert_eq!(decoded, E::Other(0x0102)); +} + +/// Regression: signed 24-bit id_pat enum must round-trip negative values on little-endian. +/// A fixed negative id (-8388608) and a negative range value (-100) must both +/// encode and decode correctly. +#[test] +#[cfg(feature = "bits")] +fn test_id_pat_signed_i24_negative_roundtrip_le() { + #[derive(PartialEq, Debug, DekuRead, DekuWrite)] + #[deku(id_type = "i32", bits = "24", endian = "little")] + enum Signed24LE { + #[deku(id = "-8388608")] + Fixed, + #[deku(id_pat = "-8388607..=8388607")] + Other(i32), + } + + let cases = [ + Signed24LE::Fixed, + Signed24LE::Other(-100), + Signed24LE::Other(100), + ]; + for val in &cases { + let bytes = val.to_bytes().unwrap(); + assert_eq!(bytes.len(), 3, "24-bit enum should encode to 3 bytes"); + let mut cursor = Cursor::new(&bytes[..]); + let (_, decoded) = Signed24LE::from_reader((&mut cursor, 0)).unwrap(); + assert_eq!(val, &decoded, "LE round-trip failed for {val:?}"); + } +} + +/// Same as above but big-endian — exercises both endian threading and sign-extension. +#[test] +#[cfg(feature = "bits")] +fn test_id_pat_signed_i24_negative_roundtrip_be() { + #[derive(PartialEq, Debug, DekuRead, DekuWrite)] + #[deku(id_type = "i32", bits = "24", endian = "big")] + enum Signed24BE { + #[deku(id = "-8388608")] + Fixed, + #[deku(id_pat = "-8388607..=8388607")] + Other(i32), + } + + let cases = [ + Signed24BE::Fixed, + Signed24BE::Other(-100), + Signed24BE::Other(100), + ]; + for val in &cases { + let bytes = val.to_bytes().unwrap(); + assert_eq!(bytes.len(), 3, "24-bit enum should encode to 3 bytes"); + let mut cursor = Cursor::new(&bytes[..]); + let (_, decoded) = Signed24BE::from_reader((&mut cursor, 0)).unwrap(); + assert_eq!(val, &decoded, "BE round-trip failed for {val:?}"); + } +} diff --git a/tests/test_regression.rs b/tests/test_regression.rs index 975fda6e0..a270bd1e7 100644 --- a/tests/test_regression.rs +++ b/tests/test_regression.rs @@ -451,3 +451,29 @@ fn issue_525() { assert_eq!(test.0, (ret, 0)); assert_eq!(test.1, TestSeek { a: 0, b: 2, c: 3 }); } + +// A `bytes` expression evaluating to 0 at runtime must read nothing and yield 0, +// rather than overflowing the sign-extension shift. +// +// https://github.com/sharksforarms/deku/pull/664#issuecomment-5162310570 +#[test] +fn pr_664_runtime_zero_bytes() { + #[derive(Debug, PartialEq, DekuRead)] + struct Dynamic { + n: u8, + #[deku(bytes = "*n as usize")] + a: i32, + } + + // n = 0: no bytes are read for `a`, the trailing bytes are left untouched + let mut cursor = std::io::Cursor::new([0x00_u8, 0x01, 0x02]); + let (amt_read, val) = Dynamic::from_reader((&mut cursor, 0)).unwrap(); + assert_eq!(8, amt_read); + assert_eq!(Dynamic { n: 0, a: 0 }, val); + + // n = 1: the single byte read is still sign-extended + let mut cursor = std::io::Cursor::new([0x01_u8, 0x9C]); + let (amt_read, val) = Dynamic::from_reader((&mut cursor, 0)).unwrap(); + assert_eq!(16, amt_read); + assert_eq!(Dynamic { n: 1, a: -100 }, val); +}