Skip to content

components: process extract memory initializers for adapters.#4605

Closed
peterhuene wants to merge 2 commits intobytecodealliance:mainfrom
peterhuene:fix-extract-memory-initializer
Closed

components: process extract memory initializers for adapters.#4605
peterhuene wants to merge 2 commits intobytecodealliance:mainfrom
peterhuene:fix-extract-memory-initializer

Conversation

@peterhuene
Copy link
Copy Markdown
Member

This PR fixes an issue where the ExtractMemory initializer wasn't
processed for adapters, leading to incorrect runtime instance indexes in
the initializer resulting from the insertion of adapter instances.

Ultimately this would either resolve the memory from the wrong instance
or panic if the referenced instance didn't export a memory when
instantiating the component.

The fix is to fixup the memory export's instance when processing global
initializers.

This commit fixes an issue where the `ExtractMemory` initializer wasn't
processed for adapters, leading to incorrect runtime instance indexes in
the initializer resulting from the insertion of adapter instances.

Ultimately this would either resolve the memory from the wrong instance
or panic if the referenced instance didn't export a memory when
instantiating the component.

The fix is to fixup the memory export's instance when processing global
initializers.
@peterhuene peterhuene requested a review from alexcrichton August 4, 2022 01:21
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Aug 4, 2022
@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 4, 2022

Subscribe to Label Action

cc @peterhuene

Details This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Copy Markdown
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Oh dear, thanks for this! This makes me even more confident in the approach of #4597 which I think would also fix this issue (or at least I hope). That being said this is obviously correct and I think it's fine to merge in the meantime. Could you add a test though which exercises this?

@peterhuene
Copy link
Copy Markdown
Member Author

I'll see about coming up with a minimal test case that exposes this bug (it was found via a composition of multiple Rust components, which makes it hard to use as a small test case).

@alexcrichton
Copy link
Copy Markdown
Member

If you want you can throw a large *.wast file in and I can try to reduce it later, no need for it to be minimal in the initial pass

@peterhuene
Copy link
Copy Markdown
Member Author

Closing in favor of #4622.

@peterhuene peterhuene closed this Aug 5, 2022
@peterhuene peterhuene deleted the fix-extract-memory-initializer branch August 5, 2022 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wasmtime:api Related to the API of the `wasmtime` crate itself

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants