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: copy buffers read from wasm memory #22

Merged
merged 2 commits into from
Sep 21, 2023
Merged

Conversation

mhmd-azeez
Copy link
Collaborator

By default, Wazero's api.Memory.Read returns a view of the underlying memory:

Write-through

This returns a view of the underlying memory, not a copy. This means any
writes to the slice returned are visible to Wasm, and any updates from
Wasm are visible reading the returned slice.

For example:
buf, _ = memory.Read(offset, byteCount)
buf[1] = 'a' // writes through to memory, meaning Wasm code see 'a'.

If you don't intend-write through, make a copy of the returned slice.

This caused an issue for varSet because it basically set the value of the variable to a view of the underlying memory. And before every call, we call extism_reset to reset the wasm memory. So this would basically zero out the values of all variables.

We have two choices:

  1. Copy the buffer in ReadBytes which is what I have done in this PR
  2. Copy the buffer in varSet. This allows for fine grain control over reading wasm memory, but it might create similar (hard to track) bugs in the future

@mhmd-azeez mhmd-azeez requested review from bhelx and zshipko September 20, 2023 13:21
@mhmd-azeez
Copy link
Collaborator Author

I have also fixed the JS SDK which had the same problem, the Rust SDK doesn't have this problem

Copy link
Contributor

@zshipko zshipko left a comment

Choose a reason for hiding this comment

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

Good catch!

@mhmd-azeez
Copy link
Collaborator Author

It was all thanks to @bhelx's new count vowels plugin

@mhmd-azeez mhmd-azeez merged commit 9dae66a into main Sep 21, 2023
3 checks passed
@mhmd-azeez mhmd-azeez deleted the fix/multiple-calls branch September 21, 2023 10:44
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