Skip to content

fix(wincode): stop serializing uninitialized podstring/podvec capacity - #13

Open
mooncitydev wants to merge 1 commit into
blueshift-gg:mainfrom
mooncitydev:fix/wincode-serialize-uninit-capacity
Open

fix(wincode): stop serializing uninitialized podstring/podvec capacity#13
mooncitydev wants to merge 1 commit into
blueshift-gg:mainfrom
mooncitydev:fix/wincode-serialize-uninit-capacity

Conversation

@mooncitydev

Copy link
Copy Markdown
Contributor

hey team, its moondev. was poking around the wincode serialization path and think i hit a real soundness bug, so heres a fix.

PodString and PodVec keep their data in a [MaybeUninit<_>; N] capacity array, so only the length prefix and the first len bytes are ever initialized. the SchemaWrite impls in pod/wincode.rs cast the whole struct to &[u8] over size_of::<Self>() and hand that to the writer, which reads the uninitialized capacity past len.

three things wrong with that:

  • its UB. reading uninitialized memory is undefined behaviour, and miri flags it right away.
  • it leaks whatever stale stack/heap bytes were sitting in the unused capacity straight into the serialized output.
  • the output isnt deterministic. two equal values can encode to different bytes depending on what was left in the capacity, which breaks the byte stability the crate otherwise upholds (theres a fixed_byte_stability test asserting exactly that for the on-chain path).

repro under miri before the fix: serialize a PodString<32> holding "hi" and then touch the output. serialize copies the uninit capacity into the output buffer, so miri fires the moment those bytes are read:

error: Undefined Behavior: reading memory at alloc[0x3..0x4], but memory is uninitialized
alloc (Rust heap, size: 33, align: 1) {
    0x00 | 02 68 69 __ __ __ ...   // len=2, "hi", then 30 uninitialized bytes
}

the fix writes the initialized prefix + active bytes and then zero-fills the rest of the capacity. the wire image is still exactly size_of::<Self>() bytes so the format doesnt change, its just fully initialized and deterministic now. the read side already only looks at the first len bytes so it didnt need touching.

added tests/wincode_serialize.rs with the zero-padding layout, a roundtrip, and a determinism check (a string set long then truncated must serialize the same as one set short, and the truncated bytes must not leak). green on stable and clean under miri, and cargo fmt / clippy --all-targets --all-features -D warnings / test --all --all-features all pass.

left PodOption as is since its value slot is always initialized (none() zeroes it, some() writes it). cheers

PodString and PodVec keep their tail in a [MaybeUninit<_>; N] capacity array,
so only the length prefix and the first `len` bytes are ever initialized. the
SchemaWrite impls cast the whole struct to &[u8] over size_of::<Self>(), which
reads the uninitialized capacity past `len`.

that is UB (Miri flags it as a read of uninitialized memory), it leaks stale
stack/heap bytes into the serialized output, and it makes serialization
non-deterministic: two equal values can encode to different bytes depending on
whatever was left in the capacity, which breaks the byte stability the crate
otherwise upholds.

write the initialized prefix + active bytes and zero-fill the rest of the
capacity, so the wire image is still a fixed size_of::<Self>() bytes but fully
initialized and deterministic. the read side is unchanged since it only ever
looks at the first `len` bytes.
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.

1 participant