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

Fix: Start Wasm table for the RTS at offset >= 1 #4685

Merged
merged 4 commits into from
Sep 11, 2024
Merged

Conversation

luc-blaeser
Copy link
Contributor

@luc-blaeser luc-blaeser commented Sep 6, 2024

Rust requires a table offset of at least 1 as the element index 0 is considered invalid and causes a debug null check to panic when called. On the other hand, elem[0] can be used by the Motoko backend code, as correct Rust-generated Wasm code does not call elem[0].

This bug is independent of wasm32 and wasm64.

This issue has been observed in https://github.com/dfinity/motoko/actions/runs/10703077671/job/29672766216?pr=4683 and happened only on Linux and only under nix-build (not nix-shell).

@luc-blaeser luc-blaeser self-assigned this Sep 6, 2024
@luc-blaeser luc-blaeser added the Bug Something isn't working label Sep 6, 2024
Copy link

github-actions bot commented Sep 6, 2024

Comparing from 5b36b8b to 6080b3f:
In terms of gas, no changes are observed in 5 tests.
In terms of size, no changes are observed in 5 tests.

@luc-blaeser luc-blaeser requested review from crusso and ggreif September 9, 2024 08:07
@luc-blaeser luc-blaeser marked this pull request as ready for review September 9, 2024 08:07
src/linking/linkModule.ml Outdated Show resolved Hide resolved
Copy link
Contributor

@ggreif ggreif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why didn't the bytecode-verifier in the replica catch this? Do we have Motoko-compiled canisters out there with this bug baked-in?

@luc-blaeser
Copy link
Contributor Author

Why didn't the bytecode-verifier in the replica catch this? Do we have Motoko-compiled canisters out there with this bug baked-in?

It is actually only a requirement by Rust, not Wasm. It only affects debug Rust compilation AFAIK. I believe people probably do not release debug Motoko code. So, probably it is okay.

Co-authored-by: Gabor Greif <gabor@dfinity.org>
Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ggreif ggreif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me!

@luc-blaeser
Copy link
Contributor Author

Thank you for the review, Claudio and Gabor!

@luc-blaeser luc-blaeser added the automerge-squash When ready, merge (using squash) label Sep 11, 2024
@mergify mergify bot merged commit 983c988 into master Sep 11, 2024
10 checks passed
@mergify mergify bot deleted the luc/fix-table-base branch September 11, 2024 11:13
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants