Exception on multithreaded wasm

[chemicstry]

I'm building a project (bevy) on multithreaded wasm via web workers and this library crashes with exception:

Uncaught (in promise) TypeError: Failed to execute 'getRandomValues' on 'Crypto': The provided ArrayBufferView value must not be shared.

Indeed the spec does not allow generating random numbers into a memory backed by SharedArrayBuffer.

I managed to fix this issue by generating numbers into an intermediate buffer allocated by javascript:

for chunk in dest.chunks_mut(65536) {
    let arr = js_sys::Uint8Array::new_with_length(chunk.len() as u32);
    n.get_random_values(&arr);
    arr.copy_to(chunk);
}

This introduces some overhead, but I'm not sure if there is a standard way to check whether rust's memory is backed by SharedArrayBuffer.

I can submit a PR for this, so let me know how you want me to proceed.

[newpavlov]

Some discussion on the matter: tc39/proposal-csprng#6

Allocating a buffer for each chunk via JS looks like a bit too much overhead to make this workaround a default option. But IIUC allocating a temporary buffer on the Rust side will not work either, since compiler can eliminate "needless" copies.

My first reaction is to suggest not using a shared array like this and instead perform copy of generated random values in a higher level code, but I don't have a strong opinion on this matter. If it will be decided to leave code as-is, at the very least we should document this behavior.

[gregkatz]

@chemicstry, would you mind sharing your fork? I'm not sure where the code you've shared above should go, and I'm hitting this as well. I don't care about the performance overhead for my personal project.

[josephlr]

@chemicstry I would like some more info on how this is happening. Looking at the js_sys::SharedArrayBuffer docs there doesn't seem a way to turn that object into a Rust &mut [u8]. How is this being done?

Perhaps there just shouldn't be a way to turn a SharedArrayBuffer into a &mut [u8] without copying. Our call to getRandomValues isn't going to be the only API that breaks, so maybe this should be fixed at a higher level. This seems reasonable, given that there are also a ton of security issues around SharedArrayBuffer as well.

Allocating a buffer for each chunk via JS looks like a bit too much overhead to make this workaround a default option. But IIUC allocating a temporary buffer on the Rust side will not work either, since compiler can eliminate "needless" copies.

@newpavlov, I don't think overhead should be too big of a deal here (as users should be using a CSPRNG if performance matters). I'm just wondering if the following code would work:

// arr could also be on the stack
let mut arr = vec![0; 65536];
for chunk in dest.chunks_mut(65535) {
    let mut s = &mut arr[..chunk.len()];
    n.get_random_values(s); // Error checking ...
    chunk.copy_from_slice(s);
}

I don't think it would be legal to optimize away the fact that the buffer being passed to get_random_values isn't the same buffer that's passed into the function. This allowed optimizations would be similar to an extern "C" function, where eliding the stack allocation would violate the "as-if" rule. See an example here: https://rust.godbolt.org/z/8nbh31 (if fill is extern, the stack-based array is used, if fill is Rust code, the stack-based array is completely eliminated).

[chemicstry]

When building for multithreaded wasm32-unknown-unknown target all rust memory (heap and stack(?)) is backed by a SharedArrayBuffer hence it can be shared with web workers. I'm not too familiar with rust internals on how it's actually implemented, but I believe it's in std memory allocator implementation. So anything allocated in rust will be inside a SharedArrayBuffer. Note that at the moment shared wasm memory is only supported by nightly rust. You can see raytrace-parallel example in wasm-bindgen docs.

Using js_sys::Uint8Array circumvents this problem, because it exits wasm and creates buffer in js shim file of wasm-bindgen.

I'm currently on the phone, but I can give a more concrete example tomorrow. I will also test if it works with stack allocated slice.

[josephlr]

When building for multithreaded wasm32-unknown-unknown target all rust memory (heap and stack(?)) is backed by a SharedArrayBuffer hence it can be shared with web workers.

Ok that makes sense. I'm worried that this decision will break a bunch of stuff (not just us), given the restrictions on various APIs and their use of SharedArrayBuffer. It seems like normal memory should be used and only memory that "must" be shared should use SharedArrayBuffer, but I'm not familiar with the multithreaded wasm stuff.

I will also test if it works with stack allocated slice.

Sounds good, let me know what you find.

[newpavlov]

@josephlr

I'm just wondering if the following code would work:

Yeah, I also initially thought about a similar approach, but was not sure if it will not be optimized out.

BTW we probably should use a stack allocated buffer with a significantly reduced chunk size (e.g. 256 bytes) instead of the vector, since the main use-case for getrandom is generation of PRNG seeds and cryptographic keys, such change should not be noticeable in most applications.

[chemicstry]

@newpavlov, I don't think overhead should be too big of a deal here (as users should be using a CSPRNG if performance matters). I'm just wondering if the following code would work:

// arr could also be on the stack
let mut arr = vec![0; 65536];
for chunk in dest.chunks_mut(65535) {
    let mut s = &mut arr[..chunk.len()];
    n.get_random_values(s); // Error checking ...
    chunk.copy_from_slice(s);
}

As expected, this does not work either. I tried with vec and with stack allocated slice.

You can't specify (at least not trivially) how memory in wasm is allocated, because that's just how wasm works. In single thread case all memory is backed by ArrayBuffer, which acts like a system memory in native OS and everything is stored in it. When you compile with atomics, it's automatically changed to SharedArrayBuffer. I don't think you can use different memory backend without additional API in wasm-bindgen or rust-lang, but I don't know much about wasm internals.

The Uint8Array method works because it is created by the javascript runtime and not wasm. The returned JsValue is a handle to memory residing somewhere in the browser, outside wasm memory region. This handle is also !Send and can panic in various ways if handled incorrectly.

@gregkatz here is a PoC fix that I have. It is based on v0.1.15 though.

[josephlr]

As expected, this does not work either. I tried with vec and with stack allocated slice.

After skimming the WASM threads proposal (which is language agnostic), it makes sense why the stack has to be a SharedArrayBuffer, as the "stack" in WASM is just a pointer to the generic Memory region (which is a SharedArrayBuffer in multithreaded WASM).

Given these restrictions, I think the best thing to do is to allocate a 32-byte (or 256-byte) non-shared buffer when initializing BrowserCrypto. Then use that non-shared buffer to get the random data and copy it to the target slice.

Note that Node doesn't seem to have these problems. crypto.randomFillSync accepts a DataView which can be backed by any sort of Buffer.

I'm not sure if there is a standard way to check whether rust's memory is backed by SharedArrayBuffer

The EMCA IsSharedArrayBuffer might work, but IDK how we would perform this check in Rust. We could also just try casting a static array to a SharedArrayBuffer, but again I don't know if that's safe or sound.

[gregkatz]

Thanks very much @chemicstry. Your fix worked for me.

[josephlr]

It looks like #165 fixed this problem, closin. Backport for v0.1 (if it happends) can be tracked separtely.