-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Use getentropy()
instead of /dev/urandom
on Emscripten
#107221
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon. Please see the contribution instructions for more information. |
Required by rust-lang/rust#107221.
Add getentropy for Emscripten Required by rust-lang/rust#107221. ~~Marked as draft because I could not find `libc-test/semver/emscripten.txt`, so it doesn't meet the checklist.~~ Fixed with commit 6a7bd43.
`/dev/urandom` is usually available on Emscripten, except when using the special `NODERAWFS` filesystem backend, which replaces all normal filesystem access with direct Node.js operations. Since this filesystem backend directly access the filesystem on the OS, it is not recommended to depend on `/dev/urandom`, especially when trying to run the Wasm binary on OSes that are not Unix-based. This can be considered a non-functional change, since Emscripten implements `/dev/urandom` in the same way as `getentropy()` when not linking with `-sNODERAWFS`.
facd1e3
to
4d40cdf
Compare
This is ready for review now that libc 0.2.140 was released. |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
These commits modify the If this was intentional then you can ignore this comment. |
Required by rust-lang/rust#107221.
lgtm @bors r+ |
⌛ Testing commit 4d40cdf with merge 8e6b904aeb2f7cd2dc46ea94a5390252d1303d9b... |
💔 Test failed - checks-actions |
This comment has been minimized.
This comment has been minimized.
`getentropy()` is available since Emscripten 2.0.5. See: emscripten-core/emscripten#12240
It looks like we need to bump the minimum supported Emscripten version to 2.0.5 for this (which is 2 months newer than 1.39.20), see commit 7b40eb7. |
Ah, ok. Let's try again... @bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (1033857): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. |
/dev/urandom
is usually available on Emscripten, except when usingthe special
NODERAWFS
filesystem backend, which replaces all normalfilesystem access with direct Node.js operations.
Since this filesystem backend directly access the filesystem on the
OS, it is not recommended to depend on
/dev/urandom
, especiallywhen trying to run the Wasm binary on OSes that are not Unix-based.
This can be considered a non-functional change, since Emscripten
implements
/dev/urandom
in the same way asgetentropy()
when notlinking with
-sNODERAWFS
.