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

wast lexer size assert not compatible with randomized struct layouts #1750

Closed
the8472 opened this issue Aug 31, 2024 · 6 comments · Fixed by #1751
Closed

wast lexer size assert not compatible with randomized struct layouts #1750

the8472 opened this issue Aug 31, 2024 · 6 comments · Fixed by #1751

Comments

@the8472
Copy link

the8472 commented Aug 31, 2024

const _: () = {
assert!(std::mem::size_of::<Token>() <= std::mem::size_of::<u64>() * 2);
};

breaks when compiling with layout randomization

If the assert is necessary for correctness then repr(C) should be used to get a deterministic layout.
If it's just an optimization then it's probably better to just check it in tests/CI.

Found this while trying to build the rust compiler with randomized layouts. rust-lang/rust#101339

@alexcrichton
Copy link
Member

Thanks for the report, and sorry for the breakage! Is this something that you'd prefer to see urgently fixed? If so I can work on getting a patch release out for rust-lang/rust to update to. (it's a bit nonstandard to do patch releases in this repo unfortunately but I can make it work)

@the8472
Copy link
Author

the8472 commented Aug 31, 2024

Not urgent, I can probably work around it for now by excluding a bunch of things from randomization.
I see that wast is updated monthly or so, that should be fine.

@the8472
Copy link
Author

the8472 commented Nov 3, 2024

Looks like that wasn't sufficient, there are another compile time size assert:

const _: () = {
let size = std::mem::size_of::<Instruction<'_>>();
let pointer = std::mem::size_of::<u64>();
assert!(size <= pointer * 11);
};

@the8472
Copy link
Author

the8472 commented Nov 3, 2024

There are more

const _: () = {
assert!(core::mem::size_of::<CoreTypeId>() <= 4);
};

generally all such asserts should be moved to tests, otherwise the crates break under layout randomization.

alexcrichton added a commit to alexcrichton/wasm-tools that referenced this issue Nov 5, 2024
Helps to address [builds with `-Zrandomize-layout`][comment]

[comment]: bytecodealliance#1750 (comment)
@alexcrichton
Copy link
Member

Oops, thanks for pointing that out! I've submitted #1894 to relax some more assertions.

To confirm though, in the second case you mentioned, that assert is technically ok to leave as a const-assert right? It's an assertion about a #[repr(C)] structure which -Zrandomize-layouts doesn't affect, right?

@the8472
Copy link
Author

the8472 commented Nov 5, 2024

Oh yeah, I didn't look at the struct declaration. That one is fine.

github-merge-queue bot pushed a commit that referenced this issue Nov 5, 2024
Helps to address [builds with `-Zrandomize-layout`][comment]

[comment]: #1750 (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