Closed scurest closed 6 years ago
Thank you! I did not know that this was possible.
I think in this case I would actually prefer crashing, rather than returning -1
. If the crypto is not available at all then the caller would probably just try again endlessly (infinite loop) or dismiss the error (using bad randomness) or crash anyway.
I'm not really familiar with emscripten. Could we put a try
-catch
around require
and throw a useful error, like "Could not import crypto module. Is it compiled into your version of Node.js?"
?
I.e. could such an exception be recovered from, for use in—say—a webserver, or will it always crash the process?
Anyway, I would like to opt for crashing if the alternative will probably cause a nasty bug.
Generally I don't think libraries should crash. They should leave the choice of what to do to their consumer. Even if the caller does want to crash, they may have cleanup they want to do. Returning -1 forever is also consistent with what will happen on Linux where there isn't getrandom
and /dev/{u,}random
doesn't exist (eg. because the right mknod
s haven't been run).
If you do want to crash though I think that should be documented in the Readme.
I don't think you can recover from a JS exception in an EM_ASM
block from C but I'm also not really familiar with Emscripten :) I asked on #emscripten but haven't gotten a response yet. You definitely can catch the JS exception from JS code that calls into the compiled C that uses randombytes, but that's a strange place to handle the error.
I guess you're right. I did make the library such that it should not crash. It's probably best to keep it that way.
However, in the rest of the code, I try to set errno
when an error occurs, to give the user a hint to what happened. Maybe do it here as well? (Unsure which: perhaps ENOPKG
?)
Otherwise I'll merge the PR nonetheless, cause it's an improvement anyway. :smile:
I added some error reporting to your patches. What do you think?
Looks good to me!
Awesome! Let's merge.
Thank you!
According to the Node docs
So I just moved the
require
inside of the try block so randombytes will return -1 instead of bringing down the caller in that case.