Use getentropy from std::random_device, avoiding FS usage of /dev/urandom #12240
Conversation
…ndom. Fixes #10068. This does *not* change the random number mechanism, just it avoids going through the FS
| getentropy: function(buffer, size) { | ||
| if (!_getentropy.randomDevice) { | ||
| _getentropy.randomDevice = getRandomDevice(); | ||
| } |
There was a problem hiding this comment.
Would it makes sense to do this memorization in getRandomDevice itself?
Also if you made _getentropy.randomDevice into $randomDevice then you could maybe save a few bytes?
There was a problem hiding this comment.
I tried a few ways, but it's hard to reduce the code size below what it currently is.
There was a problem hiding this comment.
But we not memoize the getRandomDevice inner function? And why not use global for the memoization?
There was a problem hiding this comment.
I see getRandomDevice as getting a random device. It returns a new function each time.
In theory each of those functions could have its own internal DRNG or such (see discussion in #10068).
Callers can memoize when it makes sense. In the FS code we do, and in getentropy it does.
I do agree that if memoizing in getRandomDevice itself were smaller, it would be worth figuring out a proper name for that different API. I can't get it any smaller, though...
system/include/compat/sys/unistd.h
Outdated
| @@ -1 +1,2 @@ | |||
| #include <unistd.h> | |||
| #include <random.h> // for getentropy() | |||
There was a problem hiding this comment.
Is this file sys/unistd.h ever actually included by anyone? (As opposed to unistd.h itself which lives elsewhere?)
There was a problem hiding this comment.
Oh, I meant to add it to compat/unistd.h, so yes, this was the wrong place.
Maybe it doesn't need to be added anywhere? The docs say it should be in unistd, but the usage in libc++ is for random.h. I'll remove this for now, we can add more later I guess.
There was a problem hiding this comment.
If it goes anywhere it should probably be in ./system/include/libc/unistd.h I guess?
There was a problem hiding this comment.
Looks like libc++ includes #include <sys/random.h> directly so making it part of unistd.h too is not necessary in this case.
There was a problem hiding this comment.
Yeah, looks like sys/random is enough.
| @@ -0,0 +1,9 @@ | |||
| #ifdef __cplusplus | |||
There was a problem hiding this comment.
Maybe add a comment about why we are adding this.. how it helps?
There was a problem hiding this comment.
I don't understand. The __cplusplus pattern? Lots of headers have this, like these siblings in the same dir:
system/include/compat/malloc.h
system/include/compat/netdb.h
system/include/compat/stdarg.h
system/include/compat/stdlib.h
system/include/compat/string.h
system/include/compat/time.h
system/include/compat/xlocale.h
There was a problem hiding this comment.
Sorry no I meant a comment about why this header is added by emscripten when its not part of musl? What is the purpose of adding getransom? I guess that answer is something like "to get ransom bytes without depending on the filesytem stuff which is good for code size and complexity in the generated output" .. or something along those lines :)
There was a problem hiding this comment.
Ok, adding now.
The bigger question is maybe why isn't this in musl, given it seems to be a Linux unistd.h thing..? Looks like musl has added it meanwhile actually, ifduyue/musl@82f1768#diff-2fc78454dd4e341aace02870b1e0c8c9 - but using a syscall, which would adds some unnecessary redirection for us, and it's not where libc++ looks for it (in unistd as opposed to sys/random).
| @@ -308,7 +308,7 @@ | |||
| // random data even when using sandboxing mechanisms such as chroots, | |||
| // Capsicum, etc. | |||
| # define _LIBCPP_USING_ARC4_RANDOM | |||
| #elif defined(__Fuchsia__) || defined(__wasi__) | |||
| #elif defined(__Fuchsia__) || defined(__wasi__) || defined(__EMSCRIPTEN__) | |||
There was a problem hiding this comment.
Mention in system/lib/libcxx/readme.txt?
There was a problem hiding this comment.
Good idea, added. Also I added a mention in the changelog.
`getentropy()` is available since Emscripten 2.0.5. See: emscripten-core/emscripten#12240
This does not change the random number mechanism, it just avoids going through the FS
in order to get random values.
getRandomDeviceis refactored out of the FS code withoutchange into a global helper.
By avoiding the FS this is faster, in particular in pthreads builds all FS operations are
proxied, so this PR avoids that overhead.
As a benefit this makes us do the same as WASI, so it removes a difference.
Also make the test for this actually get a random number, and not just set up the device
(that used to open the FD, so the test wasn't useless before).
Fixes #10068.