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

Handle extended const segment offsets in the fuzzer #6382

Merged
merged 3 commits into from
Mar 7, 2024

Conversation

tlively
Copy link
Member

@tlively tlively commented Mar 7, 2024

The fuzzer already had logic to remove all references to non-imported globals
from global initializers and data segment offsets, but it was missing for
element segment offsets. Add it, and also add a missing check line for the new
test that uncovered this bug as initial fuzzer input.

@tlively tlively requested a review from kripken March 7, 2024 02:35
@tlively
Copy link
Member Author

tlively commented Mar 7, 2024

@tlively tlively mentioned this pull request Mar 7, 2024
// If the offset is a global that was imported (which is ok) but no
// longer is (not ok) we need to change that.
if (auto* offset = segment->offset->dynCast<GlobalGet>()) {
if (!wasm.getGlobal(offset->name)->imported()) {
Copy link
Member

Choose a reason for hiding this comment

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

Why did the check for ->imported() go away? I think we only need to zero it out of there is a global.get that is not imported.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I see that finalizeMemory does do this check, but with a note that imported globals are never encountered. Indeed, setupGlobals removes all imports. I guess I can follow finalizeMemory's lead here.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Another option might be to assert on not seeing an import there - lgtm either way.

@tlively
Copy link
Member Author

tlively commented Mar 7, 2024

Oh yes, assertions make much more sense. Will change both cases.

Base automatically changed from eh-fuzzer-fix to main March 7, 2024 16:20
The fuzzer already had logic to remove all references to non-imported globals
from global initializers and data segment offsets, but it was missing for
element segment offsets. Add it, and also add a missing check line for the new
test that uncovered this bug as initial fuzzer input.
@tlively tlively force-pushed the fuzzer-fix-segment-offsets branch from ad7c8d3 to 979b615 Compare March 7, 2024 16:23
@tlively tlively merged commit adca3a1 into main Mar 7, 2024
15 checks passed
@tlively tlively deleted the fuzzer-fix-segment-offsets branch March 7, 2024 16:57
@tlively
Copy link
Member Author

tlively commented Mar 7, 2024

Merge activity

@gkdn gkdn mentioned this pull request Aug 31, 2024
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.

2 participants