Skip to content

Various UB Fixes - #317

Merged
phip1611 merged 5 commits into
mainfrom
ub
Sep 2, 2026
Merged

Various UB Fixes#317
phip1611 merged 5 commits into
mainfrom
ub

Conversation

@phip1611

@phip1611 phip1611 commented Sep 2, 2026

Copy link
Copy Markdown
Member

No description provided.

@phip1611
phip1611 force-pushed the ub branch 2 times, most recently from bb67996 to a1c1d41 Compare September 2, 2026 11:41
Comment thread multiboot2/src/network.rs Outdated
phip1611

This comment was marked as outdated.

MaybeDynSized::as_bytes previously covered size_of_val(self) bytes and
thus included the implicit trailing padding that the Rust memory layout
adds beyond the structure size reported in the header. For
stack-constructed sized tags, that padding is uninitialized memory, so
reading the returned slice was undefined behavior (caught by Miri).
Zeroing the padding in the constructors is no fix: the language resets
padding bytes to uninitialized on every typed copy, including returning
the value from the constructor.

Instead, as_bytes now returns a plain byte slice covering exactly the
reported structure size, clamped to the allocation, so uninitialized
padding is never exposed.

The tag alignment in the built structures previously came from the
padded as_bytes slices. Both builders now serialize their tags into a
byte buffer and fill it with explicit zeroes up to the next 8-byte
boundary after each tag, as mandated by the Multiboot2 spec. The built
structures are byte-wise identical, except that the inter-tag padding
is now guaranteed to be zeroed instead of copied from uninitialized
memory.
As a side effect of the previous commit, payload() and therefore
clone_dyn() now operate on the exact payload: cloning a DST whose size
is no multiple of the alignment previously grew the reported size to
the padded allocation size; now the size round-trips exactly. Lock this
in with a test using a payload whose tag size is no multiple of the
alignment.
…ytes

Regression test for the uninitialized-padding fixes: reading every byte
of the built MBI is defined behavior only if all serialized tags are
fully initialized. Miri catches any regression.
Regression test for the uninitialized-padding fixes: reading every byte
of the built header is defined behavior only if all serialized tags are
fully initialized. Miri catches any regression.
One representative test for all stack-constructed sized tags with
implicit trailing padding: the constructor plus as_bytes() must expose
exactly the reported tag size and only initialized bytes. Miri verifies
the absence of undefined behavior. ApmTag is the tag with the most
fields and the largest gap between spec size (28) and Rust type size
(32).
@phip1611
phip1611 enabled auto-merge September 2, 2026 14:00
@phip1611 phip1611 self-assigned this Sep 2, 2026
@phip1611
phip1611 added this pull request to the merge queue Sep 2, 2026
Merged via the queue into main with commit ca510a0 Sep 2, 2026
29 checks passed
@phip1611
phip1611 deleted the ub branch September 2, 2026 14:06
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