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

Make MagicBuffer and ZeroCopyBuf work with SharedArrayBuffers #12678

Open
andreubotella opened this issue Jun 22, 2021 · 3 comments
Open

Make MagicBuffer and ZeroCopyBuf work with SharedArrayBuffers #12678

andreubotella opened this issue Jun 22, 2021 · 3 comments
Assignees
Labels
ext/web related to the ext/web crate feat new feature (which has been agreed to/accepted)

Comments

@andreubotella
Copy link
Contributor

#11040 is currently in the process of implementing the ability to share SharedArrayBuffers across workers, and code implemented in JS and WASM will be able to immediately benefit from it. However, as per #11040 (comment), many of the ops that use MagicBuffer are written assuming that the buffer can only be modified from the current thread/worker, and need to be reviewed and fixed before those APIs can be used with shared memory. Even worse, as per denoland/rusty_v8#711 it seems that the Rust binding for BackingStore is unsound when the backing store is shared, so ZeroCopyBuf would be unsound too.

Therefore, it seems like we should disable the deserialization of shared typed arrays into MagicBuffers for the time being, and at some later point we can figure how to rewrite MagicBuffer and ZeroCopyBuf using the new BackingStore API.

andreubotella referenced this issue in andreubotella/serde_v8 Jun 23, 2021
@andreubotella
Copy link
Contributor Author

andreubotella commented Jun 26, 2021

I've been thinking some more about this, and even in the case where the BackingStore isn't shared, the implementation of ZeroCopyBuf is still unsound:

const buffer = new Uint8Array(1024);
Deno.core.opSync("op_zerocopy_alias_test", buffer, buffer);
fn op_zerocopy_alias_test(
  _state: &mut OpState,
  mut buf1: serde_v8::Buffer,
  mut buf2: serde_v8::Buffer,
) -> Result<(), AnyError> {
  let mut_slice1: &mut [u8] = &mut *buf1;
  let mut_slice2: &mut [u8] = &mut *buf2;
  println!("Address of mut_slice1: {:?}", mut_slice1.as_ptr());
  println!("Address of mut_slice2: {:?}", mut_slice2.as_ptr());
  Ok(())
}

So ZeroCopyBuf's API allows breaking Rust's "aliasable XOR mutable" validity constraint even in single-threaded code, and so the API would have to be reconsidered anyway. But since denoland/rusty_v8#711 would already need a change in the API, it's best to consider both together.

Note that here using two v8::SharedRef<v8::BackingStore> that pointed to the same BackingStore would not cause UB, since they deref to &[Cell<u8>] rather than a mutable slice. The unsafety is introduced in get_backing_store_slice and get_backing_store_slice_mut.

It isn't easy to trigger this UB in Deno, since AFAIK no ops take two buffers, but some runs of this (very contrived) code should be able to trigger it:

const buffer = new Uint8Array(1024);

// We don't await!
fetch("https://example.com", { method: "POST", body: buffer });

new TextEncoder().encodeInto("some string", buffer);

@stale
Copy link

stale bot commented Jan 7, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 7, 2022
@kitsonk kitsonk added ext/web related to the ext/web crate feat new feature (which has been agreed to/accepted) labels Jan 7, 2022
@stale stale bot closed this as completed Jan 14, 2022
@kitsonk
Copy link
Contributor

kitsonk commented Jan 14, 2022

Stalebot, you are supposed to close feat tags! 😠

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext/web related to the ext/web crate feat new feature (which has been agreed to/accepted)
Projects
None yet
Development

No branches or pull requests

3 participants