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 multithreaded wasm crash (solves #164) #165

Merged
merged 5 commits into from
Oct 26, 2020

Conversation

chemicstry
Copy link
Contributor

This solves #164 by using a preallocated Uint8Array for calling crypto.getRandomValues.

The stdweb implementation seems fine, as it already uses Uint8Array.

Should this fix be backported to 0.15? There are a lot of crates that still use that version.

@chemicstry
Copy link
Contributor Author

The CI seems to have failed due to unrelated reason. Can someone rerun?

Copy link
Member

@josephlr josephlr left a comment

Choose a reason for hiding this comment

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

Looks good, a few minor changes. Also, now that we are depending on js-sys can you add a TODO where we use Global and Self_ to replace that functionality with js_sys::global()

Comment on lines 19 to 24
struct BrowserCryptoContext {
crypto: BrowserCrypto,
// A temporary buffer backed by browser memory to avoid multithreaded wasm exception. See issue #164
buf: Uint8Array,
}

Copy link
Member

@josephlr josephlr Oct 16, 2020

Choose a reason for hiding this comment

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

Instead of having this structure, we could just do:

enum RngSource {
    Node(NodeCrypto),
    Browser(BrowserCrypto, Uint8Array),
}

and then add the comment about using the temporary buffer when we actually use the temp buffer:

RngSource::Browser(crypto, buf) => {
    // getRandomValues does not work with all types of WASM memory, so
    // we initially write to browser memory to avoid an exception.
    for chunk in dest.chunks_mut(BROWSER_CRYPTO_BUFFER_SIZE) {
        ...
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the late response, but do you still want me to fix this? Since you force pushed to my branch and already approved it

Copy link
Member

Choose a reason for hiding this comment

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

Yes, fixing this would be preferable (I can also do it when i have time).

Comment on lines 76 to 80
let buf = Uint8Array::new_with_length(BROWSER_CRYPTO_BUFFER_SIZE as u32);

let ctx = BrowserCryptoContext { crypto, buf };

return Ok(RngSource::Browser(ctx));
Copy link
Member

Choose a reason for hiding this comment

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

This then becomes:

let buf = Uint8Array::new_with_length(BROWSER_CRYPTO_BUFFER_SIZE as u32);
return Ok(RngSource::Browser(crypto, buf));

src/wasm-bindgen.rs Show resolved Hide resolved
@josephlr
Copy link
Member

Should this fix be backported to 0.15? There are a lot of crates that still use that version.

Ya that should be fine, once we make sure this works, I'll backport it (maybe once we also cleanup the Self usage).

chemicstry and others added 2 commits October 23, 2020 14:49
Signed-off-by: Joe Richey <[email protected]>
@josephlr josephlr merged commit c29dd5f into rust-random:master Oct 26, 2020
josephlr added a commit to josephlr/getrandom that referenced this pull request Oct 28, 2020
Solves rust-random#164 for the v0.1 branch

Signed-off-by: Joe Richey <[email protected]>
newpavlov pushed a commit that referenced this pull request Dec 7, 2020
Solves #164 for the v0.1 branch
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