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

Change the return type of SharedMemory::data #5240

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Nov 10, 2022

This commit is an attempt at improving the safety of using the return value of the SharedMemory::data method. Previously this returned *mut [u8] which, while correct, is unwieldy and unsafe to work with. The new return value of &[UnsafeCell<u8>] has a few advantages:

  • The lifetime of the returned data is now connected to the SharedMemory itself, removing the possibility for a class of errors of accidentally using the prior *mut [u8] beyond its original lifetime.

  • It's now possibly to safely access .len() as opposed to requiring an unsafe dereference before.

  • The data internally within the slice is now what retains the unsafe bits, namely indicating that accessing any memory inside of the contents returned is unsafe but addressing it is safe.

I was inspired by the wiggle-based discussion on #5229 and felt it appropriate to apply a similar change here.

This commit is an attempt at improving the safety of using the return
value of the `SharedMemory::data` method. Previously this returned
`*mut [u8]` which, while correct, is unwieldy and unsafe to work with.
The new return value of `&[UnsafeCell<u8>]` has a few advantages:

* The lifetime of the returned data is now connected to the
  `SharedMemory` itself, removing the possibility for a class of errors
  of accidentally using the prior `*mut [u8]` beyond its original lifetime.

* It's not possibly to safely access `.len()` as opposed to requiring an
  `unsafe` dereference before.

* The data internally within the slice is now what retains the `unsafe`
  bits, namely indicating that accessing any memory inside of the
  contents returned is `unsafe` but addressing it is safe.

I was inspired by the `wiggle`-based discussion on bytecodealliance#5229 and felt it
appropriate to apply a similar change here.
@github-actions github-actions bot added fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself labels Nov 10, 2022
@github-actions
Copy link

Subscribe to Label Action

cc @fitzgen, @peterhuene

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

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

  • fitzgen: fuzzing
  • peterhuene: wasmtime:api

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

Learn more.

@fitzgen fitzgen merged commit 2be457c into bytecodealliance:main Nov 10, 2022
@alexcrichton alexcrichton deleted the shared-bit-safer branch February 22, 2023 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzzing Issues related to our fuzzing infrastructure 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