Skip to content
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

Replaced library_fs.js/random_device()'s Math.random fallback call with throw for safety #7096

Merged
merged 3 commits into from
Sep 5, 2018

Conversation

paulshapiro
Copy link
Contributor

Per discussion on #542

paulshapiro added a commit to mymonero/mymonero-core-js that referenced this pull request Sep 4, 2018
…n-core/emscripten#7096 and replaced Math.random in list rand index in monero_sendingFunds_utils
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good with minor suggestions.

Also please add yourself to AUTHORS.

@@ -1301,7 +1301,7 @@ mergeInto(LibraryManager.library, {
#endif // ENVIRONMENT_MAY_BE_NODE
} else {
// default for ES5 platforms
random_device = function() { return (Math.random()*256)|0; };
random_device = function() { throw "emscripten random_device implementation must be run on a modern browser to support safe random number generation."; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use abort("..text..") instead of throw - abort will also mark the program as halted, so an emscripten_main_loop etc. will know to stop itself next time it is called.

For the error text, I'd prefer something short, just to not increase code size too much. How about just "random_device", and in a comment (which in an optimized build would be removed) to give the extra detail?

@paulshapiro
Copy link
Contributor Author

Thanks! Updates pushed.

@kripken
Copy link
Member

kripken commented Sep 5, 2018

Perfect, thanks @paulshapiro!

@kripken kripken merged commit 6cc4c66 into emscripten-core:incoming Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants