-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Use getentropy from std::random_device, avoiding FS usage of /dev/urandom #12240
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
Changes from 2 commits
378fafa
50ddc08
163076e
db3eeb9
72ea088
021df8a
97caa07
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
#ifdef __cplusplus | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add a comment about why we are adding this.. how it helps? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand. The
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
extern "C" { | ||
#endif | ||
|
||
int getentropy(void *buffer, size_t length); | ||
|
||
#ifdef __cplusplus | ||
} | ||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
#include <unistd.h> | ||
#include <random.h> // for getentropy() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I meant to add it to 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it goes anywhere it should probably be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like libc++ includes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, looks like sys/random is enough. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mention in system/lib/libcxx/readme.txt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, added. Also I added a mention in the changelog. |
||
# define _LIBCPP_USING_GETENTROPY | ||
#elif defined(__native_client__) | ||
// NaCl's sandbox (which PNaCl also runs in) doesn't allow filesystem access, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we not memoize the getRandomDevice inner function? And why not use global for the memoization?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...