Currently, we do not check for errors when reading / opening files in WTF::cryptographicallyRandomValuesFromOS. This should be remedied by checking if an EAGAIN or EINTR has occurred and continuing to get more data.
Created attachment 255857 [details] Patch
Created attachment 255860 [details] Patch Changed the name of the crashing functions to make it clear they were related to /dev/urandom
rdar://problem/21552607
Comment on attachment 255860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255860&action=review > Source/WTF/wtf/OSRandomSource.cpp:53 > +NEVER_INLINE NO_RETURN_DUE_TO_CRASH static void crashUnableToOpenURandom() > +{ > + CRASH(); > +} > + > +NEVER_INLINE NO_RETURN_DUE_TO_CRASH static void crashUnableToReadFromURandom() > +{ > + CRASH(); > +} Is there a good reason for these, isntead of just using CRASH or RELEASE_ASSERT_NOT_REACHED directly? > Source/WTF/wtf/OSRandomSource.cpp:72 > + if (currentRead < 0 && !(errno == EAGAIN || errno == EINTR)) > + crashUnableToReadFromURandom(); This seems broken - if the currentRead is -1, then we don't want to add it to amountRead.
Comment on attachment 255860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255860&action=review > Source/WTF/wtf/OSRandomSource.cpp:63 > + int fd; > + // We need to check for both EAGAIN and EINTR since on some systems /dev/urandom > + // is blocking and on others it is non-blocking. > + do { > + fd = open("/dev/urandom", O_RDONLY, 0); > + } while (fd < 0 && (errno == EAGAIN || errno == EINTR)); I don't think there is a need to check for the EAGAIN and EINTR cases for open(). EAGAIN is documented to only happen for slave ptys and EINTR due to a signal.
Created attachment 255864 [details] Patch Fixed issue with currentRead being < 0
Comment on attachment 255864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255864&action=review > Source/WTF/wtf/OSRandomSource.cpp:63 > + int fd; > + // We need to check for both EAGAIN and EINTR since on some systems /dev/urandom > + // is blocking and on others it is non-blocking. > + do { > + fd = open("/dev/urandom", O_RDONLY, 0); > + } while (fd < 0 && (errno == EAGAIN || errno == EINTR)); Remove this loop. I don't think there is a need to check for the EAGAIN and EINTR cases for open(). EAGAIN is documented to only happen for slave ptys and EINTR due to a signal.
Comment on attachment 255860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255860&action=review >> Source/WTF/wtf/OSRandomSource.cpp:53 >> +} > > Is there a good reason for these, isntead of just using CRASH or RELEASE_ASSERT_NOT_REACHED directly? I requested this so that we get back definitive crash traces that differentiate the one kind of failure from the other. Currently, our crash traces do not tell us which failure happened. > Source/WTF/wtf/OSRandomSource.cpp:70 > + ssize_t currentRead = read(fd, buffer+amountRead, length-amountRead); Our style requires spacing around + and -.
Created attachment 255868 [details] Patch Removed checks for EINTR and EAGAIN from the open call.
Created attachment 255869 [details] Patch For some reason the other patch didn't update.
Comment on attachment 255869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255869&action=review > Source/WTF/wtf/OSRandomSource.cpp:67 > + if (currentRead < 0 && !(errno == EAGAIN || errno == EINTR)) I think it would be nicer to check for -1 explicitly, rather than implicitly by including all negatives in the test. The documentation states that read will return -1, 0, or a positive number, and it is nice to be precise. You can simplify the logic here a bit and avoid testing currentRead twice like so: if (currentRead == -1) { if (!(errno == EAGAIN || errno == EINTR)) crashUnableToReadFromURandom(); } else amountRead += currentRead; This helps to clearly separate the failure case from the success case.
Comment on attachment 255869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255869&action=review >> Source/WTF/wtf/OSRandomSource.cpp:67 >> + if (currentRead < 0 && !(errno == EAGAIN || errno == EINTR)) > > I think it would be nicer to check for -1 explicitly, rather than implicitly by including all negatives in the test. The documentation states that read will return -1, 0, or a positive number, and it is nice to be precise. > > You can simplify the logic here a bit and avoid testing currentRead twice like so: > > if (currentRead == -1) { > if (!(errno == EAGAIN || errno == EINTR)) > crashUnableToReadFromURandom(); > } else > amountRead += currentRead; > > This helps to clearly separate the failure case from the success case. That makes sense. Do you think the code you wrote is clearer than: if (currentRead >= 0) amountRead += currentRead; else if (!(errno == EAGAIN || errno == EINTR)) crashUnableToReadFromURandom(); I guess it depends on whether or not you think about the success or failure case first. It also, doesn't make the -1 condition explicit.
> That makes sense. Do you think the code you wrote is clearer than: > > if (currentRead >= 0) > amountRead += currentRead; > else if (!(errno == EAGAIN || errno == EINTR)) > crashUnableToReadFromURandom(); I like both. I would still use " != -1" instead of "> = 0". -1 is a sentinel value and I think it helps readability to call it out explicitly.
Created attachment 255872 [details] Patch Refactored code slightly.
Comment on attachment 255872 [details] Patch Clearing flags on attachment: 255872 Committed r186151: <http://trac.webkit.org/changeset/186151>
All reviewed patches have been landed. Closing bug.
<rdar://problem/21939126>
LOL. arc4random_buf just uses /dev/random, so we have only moved the goalposts, and we have not fixed this bug :(.