Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasmparser static TypeId assert can trigger with -Zrandomize-layout #1013

Closed
repi opened this issue May 3, 2023 · 1 comment · Fixed by #1015
Closed

wasmparser static TypeId assert can trigger with -Zrandomize-layout #1013

repi opened this issue May 3, 2023 · 1 comment · Fixed by #1015

Comments

@repi
Copy link
Contributor

repi commented May 3, 2023

The following wasmparser code can fail if compiling with nightly and -Zrandomize-layout due to a static assert triggering on the size of TypeId when the field order gets reshuffled.

/// Represents a unique identifier for a type known to a [`crate::Validator`].
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct TypeId {
    /// The index into the global list of types.
    pub(crate) index: usize,
    /// The effective type size for the type.
    ///
    /// This is stored as part of the ID to avoid having to recurse through
    /// the global type list when calculating type sizes.
    pub(crate) type_size: u32,
    /// A unique integer assigned to this type.
    ///
    /// The purpose of this field is to ensure that two different `TypeId`
    /// representations can be handed out for two different aliased types within
    /// a component that actually point to the same underlying type (as pointed
    /// to by the `index` field).
    unique_id: u32,
}

// The size of `TypeId` was seen to have a large-ish impact in #844, so this
// assert ensures that it stays relatively small.
const _: () = {
    assert!(std::mem::size_of::<TypeId>() <= 16);
};

should we increase the limit her, remove the assert, or use repr(C) on TypeId to enforce the field order of this type?

We ran into this when building our application with nightly and -Zrandomize-layout which builds all dependent crates including wasmtime and wasmparser with it as well and this failed to build. Another option could be to move the assert to a unit test so it only gets triggered in the development of wasmparser and not in usages of the crate?

@repi repi changed the title wasmparser static TypeId assert can trigger when building with -Zrandomize-layout wasmparser static TypeId assert can trigger with -Zrandomize-layout May 3, 2023
@alexcrichton
Copy link
Member

Oh dear sorry for the breakage! I think it's probably best to use #[repr(C)] here which I think should fix the issue for you. If you'd like to send a PR that'd be appreciated!

repi added a commit to EmbarkStudios/wasm-tools that referenced this issue May 5, 2023
When compiling with `-Zrandomize-layout` the fields of `TypeId` can be rearranged in a way where the type becomes larger than 16 bytes which triggered a static assert.

Resolves: bytecodealliance#1013
alexcrichton pushed a commit that referenced this issue May 15, 2023
* Freeze field layout of `TypeId`

When compiling with `-Zrandomize-layout` the fields of `TypeId` can be rearranged in a way where the type becomes larger than 16 bytes which triggered a static assert.

Resolves: #1013

* Add comment
Mossaka pushed a commit to Mossaka/wasm-tools-fork that referenced this issue May 29, 2023
* Freeze field layout of `TypeId`

When compiling with `-Zrandomize-layout` the fields of `TypeId` can be rearranged in a way where the type becomes larger than 16 bytes which triggered a static assert.

Resolves: bytecodealliance#1013

* Add comment
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 a pull request may close this issue.

2 participants