fix(wincode): stop serializing uninitialized podstring/podvec capacity - #13
Open
mooncitydev wants to merge 1 commit into
Open
fix(wincode): stop serializing uninitialized podstring/podvec capacity#13mooncitydev wants to merge 1 commit into
mooncitydev wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
hey team, its moondev. was poking around the wincode serialization path and think i hit a real soundness bug, so heres a fix.
PodStringandPodVeckeep their data in a[MaybeUninit<_>; N]capacity array, so only the length prefix and the firstlenbytes are ever initialized. theSchemaWriteimpls inpod/wincode.rscast the whole struct to&[u8]oversize_of::<Self>()and hand that to the writer, which reads the uninitialized capacity pastlen.three things wrong with that:
fixed_byte_stabilitytest 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: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 firstlenbytes so it didnt need touching.added
tests/wincode_serialize.rswith 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, andcargo fmt/clippy --all-targets --all-features -D warnings/test --all --all-featuresall pass.left
PodOptionas is since its value slot is always initialized (none()zeroes it,some()writes it). cheers