Skip to content

Fix id write endianness and sub-width signed sign-extension - #664

Merged
wcampbell0x2a merged 3 commits into
sharksforarms:masterfrom
gusanthon:fix/id-pat-endian-signext
Sep 1, 2026
Merged

Fix id write endianness and sub-width signed sign-extension#664
wcampbell0x2a merged 3 commits into
sharksforarms:masterfrom
gusanthon:fix/id-pat-endian-signext

Conversation

@gusanthon

Copy link
Copy Markdown
Contributor

Summary

Two independent correctness fixes for byte-aligned reads/writes, each with
regression tests. They're unrelated bugs that happened to surface together
while implementing a binary codec - happy to split into two PRs if you'd
prefer to review/merge them separately.


Patch A - enum id written with the wrong endianness

File: deku-derive/src/macros/deku_write.rs

On the id_pat write path, field_endian was derived from id_endian
only, ignoring both a hardcoded endian = "big" and a bound ctx endian.
As a result an enum that declares endian = "big" would still write its
id in little-endian, so the written bytes didn't match the equivalent
flat big-endian field (and didn't round-trip against the read side).

Fix: mirror the read-side precedence already used in gen_id_args
(id_endian -> enum endian) for the id_pat write path, so the write side
resolves endianness the same way the read side does.

Tests (tests/test_enum.rs):

  • an id_pat enum with endian = "big" writes the same bytes as the
    equivalent flat big-endian field
  • a signed 24-bit id_pat enum round-trips negative sentinels on both LE
    and BE

Patch B - sub-width signed reads not sign-extended

File: src/impls/primitive.rs

The (Endian, ByteSize, Order) DekuReader impl for signed types
(ImplDekuReadSignExtend macro) zero-padded the high bytes of a sub-width
byte-aligned read without propagating the sign bit. E.g. a 3-byte i32
read of -100 returned +16776092 instead of -100.

Fix: apply the same shift-based sign-extension already used in the
BitSize path, so the ByteSize path sign-extends consistently.

Tests (src/impls/primitive.rs): cover the ByteSize sign-extend path
directly for i32 and i16 sub-width reads on both endians.

@wcampbell0x2a

Copy link
Copy Markdown
Collaborator

The following would panic with this patch:

#[derive(Debug, DekuRead)]
struct Dynamic {
    n: u8,
    #[deku(bytes = "*n as usize")]
    a: i32,
}
#[test]
fn dynamic_zero() {
    // n = 0 -> ByteSize(0) at runtime
    let mut c = Cursor::new([0x00u8, 0x01, 0x02]);
    println!("{:?}", Dynamic::from_reader((&mut c, 0)));
}


}

@wcampbell0x2a wcampbell0x2a left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes, see previous comment.

Patch A (deku-derive/src/macros/deku_write.rs): Mirror gen_id_args' read-side precedence (id_endian -> enum endian) for the id_pat write path.  Previously field_endian was derived from id_endian only, ignoring a hardcoded endian="big" or a bound ctx endian, causing the enum to write the id in little-endian even when the enum declared endian="big".

Patch B (src/impls/primitive.rs): Sign-extend in the (Endian, ByteSize, Order)
DekuReader impl for signed types (ImplDekuReadSignExtend macro).  A sub-width byte-aligned signed read previously zero-padded the high bytes without propagating the sign bit, so e.g. a 3-byte i32 read of -100 returned +16776092. The fix applies the same shift-based sign-extension already used in the BitSize path.

Regression tests in tests/test_enum.rs cover:
  - id_pat enum with endian="big" writes same bytes as flat big-endian field
  - signed 24-bit id_pat enum round-trips negative sentinels on LE and BE

Regression tests in src/impls/primitive.rs cover the ByteSize sign-extend path directly for i32 and i16 sub-width reads on both endians.
@gusanthon
gusanthon force-pushed the fix/id-pat-endian-signext branch from d6e8b0e to 28ac6fc Compare August 19, 2026 10:33
@gusanthon

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @wcampbell0x2a, and good catch. Fixed in a4debfa by skipping the sign-extension when the size is 0. Your snippet gives Ok((8, Dynamic { n: 0, a: 0 })) again, same as master. Added it as a test in
tests/test_regression.rs. Also rebased onto master, so no more merge commits. Let me know if there's anything else that needs addressing.

@wcampbell0x2a
wcampbell0x2a merged commit 873ff09 into sharksforarms:master Sep 1, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants