Add OsRng wasm support #272

quininer · 2018-02-27T13:06:46Z

:)

pitdicker

Thanks for the PR!

First I have to say that I know nothing about js 😄. But left a couple of comments.

pitdicker · 2018-02-27T13:53:28Z

src/os.rs


    #[derive(Debug)]
    pub struct OsRng;

    impl OsRng {
        pub fn new() -> Result<OsRng, Error> {
-            Err(Error::new(ErrorKind::Unavailable, "not supported on WASM"))
+            Ok(OsRng)


It would be nice if we could know here if OsRng is available or not. Would it add a lot of overhead to test here?

We can check if window.crypto.getRandomValues、require("crypto") is available, but there is a certain overhead.

I'm not really sure it matters — any of the other system interfaces may succeed with new but fail on try_fill_bytes.

pitdicker · 2018-02-27T13:54:24Z

src/os.rs

+                        HEAPU8.set(array, ptr);
+                    } catch(err) {
+                        if (err instanceof ReferenceError) {
+                            let bytes = require("crypto").randomBytes(len);


Is this a fallback for Node.js or something? Could you maybe add a comment?

pitdicker · 2018-02-27T13:57:32Z

src/os.rs

+                Ok(())
+            } else {
+                let err: WebError = js!{ return @{ result }.error }.try_into().unwrap();
+                Err(Error::with_cause(ErrorKind::Other, "WASM Error", err))


You should probably give a Unavailable error if window.crypto.getRandomValues is not available (can that happen, or does every implementation of webassembly support this?). And do you know when we can get a QuotaExceededError?

b.t.w. Other just got renamed to ~~Unknown~~ Unexpected.

No, to Unexpected. But I agree, it doesn't look like this is recoverable so it should be Unavailable.

Found an answer: https://w3c.github.io/webcrypto/Overview.html#dfn-Crypto-method-getRandomValues. QuotaExceededError is returned "If the byteLength of array is greater than 65536". We maybe want to break up larger request, like the implementations of OsRng on other systems (although I would say an error is reasonable...)

pitdicker · 2018-02-27T13:57:44Z

src/os.rs

+        let mut v2 = [0u8; 1000];
+        r.fill_bytes(&mut v2);
+
+        assert_ne!(v[..], v2[..]);


dhardy

Nice work. I also no next-to-nothing about JS; is this expected to work on most WASM platforms?

dhardy · 2018-02-27T14:08:11Z

src/os.rs

+                Ok(())
+            } else {
+                let err: WebError = js!{ return @{ result }.error }.try_into().unwrap();
+                Err(Error::with_cause(ErrorKind::Other, "WASM Error", err))


No, to Unexpected. But I agree, it doesn't look like this is recoverable so it should be Unavailable.

dhardy · 2018-02-27T14:08:35Z

src/os.rs

        pub fn try_fill_bytes(&mut self, v: &mut [u8]) -> Result<(), Error> {
-            Err(Error::new(ErrorKind::Unavailable, "not supported on WASM"))
+            let len = v.len() as u32;
+            let ptr = v.as_mut_ptr() as i32;


A 32-bit pointer? Surely that's not going to work on 64-bit platforms?

Yes, it is wasm32.

Okay, then could you at least assert_eq!(size_of::<usize>(), 4)?

dhardy · 2018-02-27T14:11:18Z

src/seq.rs

-        assert_eq!(&sample_slice(&mut r, empty, 0)[..], []);
-        assert_eq!(&sample_slice(&mut r, &[42, 2, 42], 0)[..], []);
+        assert_eq!(&sample_slice(&mut r, empty, 0)[..], [0u8; 0]);
+        assert_eq!(&sample_slice(&mut r, &[42, 2, 42], 0)[..], [0u8; 0]);


Is the extra verbosity useful?

I don't know, this may seem a rust type inference bug.

Try removing the first &; the function returns a Vec so it's not necessary.

It does not work.

…to wasm

dhardy · 2018-03-01T12:08:01Z

Merged, with a tweak to the test.

Thanks @quininer.

tomaka · 2018-03-25T16:13:47Z

Sorry for the hostile tone. This PR was done with the best intentions but this shouldn't have been merged. wasm32-unknown-unknown is NOT JavaScript.

As its name says, the environment is unknown and the environment shouldn't be assumed to be the web.

Also wasm32-unknown-unknown is not stdweb or cargo-web. Instead cargo-web should be thought as a subset of wasm32-unknown-unknown.

The stdlib faces the same problem and "solves" it by panicking at runtime in every single function that requires some support from the OS or the environment in general. The stdlib also has an unstable feature flag that assumes the presence of a support function provided by the user. This crate should follow the same idiom.

quininer · 2018-03-25T16:28:44Z

I agree with @tomaka , I neglect this point.

As a remedy, we can provide a stdweb feature. or move out to become rand-stdweb.

pitdicker · 2018-03-25T17:48:17Z

An stdweb feature seems like a good choice to me. I'll take a stab at it.

dhardy · 2018-03-26T09:33:48Z

Thanks for the review @tomaka; I was hoping for extra review but didn't know who to ask.

I believe however the worst case is this will simply return Error("not supported") or cause build failure. Anyway thanks for putting us right.

quininer force-pushed the wasm branch from b105761 to 3ce7578 Compare February 27, 2018 13:25

pitdicker reviewed Feb 27, 2018

View reviewed changes

dhardy reviewed Feb 27, 2018

View reviewed changes

quininer added 2 commits February 27, 2018 23:24

Add OsRng wasm support

d539afa

Fix Other rename to Unexpected

65733b8

quininer force-pushed the wasm branch from 3ce7578 to 65733b8 Compare February 27, 2018 16:06

quininer added 2 commits February 28, 2018 09:58

Merge branch 'master' of https://github.com/rust-lang-nursery/rand in…

26d0647

…to wasm

Add check

48a8f0a

dhardy merged commit 48a8f0a into rust-random:master Mar 1, 2018

pitdicker mentioned this pull request Mar 4, 2018

add naive wasm32 support using extern date_now() #284

Closed

pitdicker mentioned this pull request Mar 25, 2018

Put stdweb dependency behind a target feature #336

Merged

dhardy mentioned this pull request May 9, 2018

Add impl code for wasm target #197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OsRng wasm support #272

Add OsRng wasm support #272

quininer commented Feb 27, 2018

pitdicker left a comment

pitdicker Feb 27, 2018

quininer Feb 27, 2018

dhardy Feb 27, 2018

pitdicker Feb 27, 2018

pitdicker Feb 27, 2018 •

edited

Loading

dhardy Feb 27, 2018

pitdicker Feb 27, 2018

pitdicker Feb 27, 2018

dhardy left a comment

dhardy Feb 27, 2018

dhardy Feb 27, 2018

quininer Feb 27, 2018

dhardy Feb 27, 2018

dhardy Feb 27, 2018

quininer Feb 27, 2018

dhardy Feb 27, 2018

quininer Feb 27, 2018

dhardy commented Mar 1, 2018

tomaka commented Mar 25, 2018

quininer commented Mar 25, 2018

pitdicker commented Mar 25, 2018

dhardy commented Mar 26, 2018

Add OsRng wasm support #272

Add OsRng wasm support #272

Conversation

quininer commented Feb 27, 2018

pitdicker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy commented Mar 1, 2018

tomaka commented Mar 25, 2018

quininer commented Mar 25, 2018

pitdicker commented Mar 25, 2018

dhardy commented Mar 26, 2018

pitdicker Feb 27, 2018 •

edited

Loading