Modify random_get to use crypto.getRandomValues #75
Conversation
src/wasi.ts
Outdated
| buffer8[buf + i] = (Math.random() * 256) | 0; | ||
| const end = buf + buf_len; | ||
|
|
||
| if (self.hasCrypto) { |
There was a problem hiding this comment.
Can you check "crypto" in globalThis instead and not cache it in an instance variable?
There was a problem hiding this comment.
I tried, but it didn't pass the tests. Looks like globalThis doesn't exist on all environments 😞
There was a problem hiding this comment.
Also, caching it is most probably faster than checking every time random_get gets called
There was a problem hiding this comment.
Did it fail in node.js? Maybe the node.js version that was used is too old? globalThis is supposed to work in all environments. It was introduced as way to unify window, self and global across the various javascript environments.
As for performance, if random_get is called rarely a tiny slowdown shouldn't matter and if it is called frequently, I would expect the JIT to optimize it down to a couple instructions max, which is much less than even the overhead of making a native call, let alone actually running the PRNG.
There was a problem hiding this comment.
I remembered it failing on some test, but it seems I was mistaken. I've pushed the uncached "crypto" in globalThis version.
|
Thanks! |
I simply modified the
random_getimplementation to usecrypto.getRandomValuesto generate the random numbers.This has the benefit that the RNG is cryptographically secure, and is probably faster that the previous implementation (though that should be benchmarked).