WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
146473
Errors in read() are not handled in WTF::cryptographicallyRandomValuesFromOS.
https://bugs.webkit.org/show_bug.cgi?id=146473
Summary
Errors in read() are not handled in WTF::cryptographicallyRandomValuesFromOS.
Keith Miller
Reported
2015-06-30 14:49:02 PDT
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.
Attachments
Patch
(3.12 KB, patch)
2015-06-30 14:54 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(3.14 KB, patch)
2015-06-30 15:00 PDT
,
Keith Miller
fpizlo
: review-
fpizlo
: commit-queue-
Details
Formatted Diff
Diff
Patch
(3.14 KB, patch)
2015-06-30 15:12 PDT
,
Keith Miller
msaboff
: review-
Details
Formatted Diff
Diff
Patch
(3.18 KB, patch)
2015-06-30 15:40 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(3.06 KB, patch)
2015-06-30 15:48 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(3.07 KB, patch)
2015-06-30 16:31 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Keith Miller
Comment 1
2015-06-30 14:54:22 PDT
Created
attachment 255857
[details]
Patch
Keith Miller
Comment 2
2015-06-30 15:00:04 PDT
Created
attachment 255860
[details]
Patch Changed the name of the crashing functions to make it clear they were related to /dev/urandom
Keith Miller
Comment 3
2015-06-30 15:03:53 PDT
rdar://problem/21552607
Filip Pizlo
Comment 4
2015-06-30 15:04:11 PDT
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.
Michael Saboff
Comment 5
2015-06-30 15:06:17 PDT
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.
Keith Miller
Comment 6
2015-06-30 15:12:32 PDT
Created
attachment 255864
[details]
Patch Fixed issue with currentRead being < 0
Michael Saboff
Comment 7
2015-06-30 15:14:56 PDT
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.
Geoffrey Garen
Comment 8
2015-06-30 15:22:11 PDT
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 -.
Keith Miller
Comment 9
2015-06-30 15:40:32 PDT
Created
attachment 255868
[details]
Patch Removed checks for EINTR and EAGAIN from the open call.
Keith Miller
Comment 10
2015-06-30 15:48:08 PDT
Created
attachment 255869
[details]
Patch For some reason the other patch didn't update.
Geoffrey Garen
Comment 11
2015-06-30 16:00:41 PDT
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.
Keith Miller
Comment 12
2015-06-30 16:09:54 PDT
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.
Geoffrey Garen
Comment 13
2015-06-30 16:23:00 PDT
> 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.
Keith Miller
Comment 14
2015-06-30 16:31:51 PDT
Created
attachment 255872
[details]
Patch Refactored code slightly.
WebKit Commit Bot
Comment 15
2015-06-30 17:37:14 PDT
Comment on
attachment 255872
[details]
Patch Clearing flags on attachment: 255872 Committed
r186151
: <
http://trac.webkit.org/changeset/186151
>
WebKit Commit Bot
Comment 16
2015-06-30 17:37:19 PDT
All reviewed patches have been landed. Closing bug.
Geoffrey Garen
Comment 17
2015-08-17 10:41:59 PDT
<
rdar://problem/21939126
>
Geoffrey Garen
Comment 18
2015-08-17 10:42:11 PDT
LOL. arc4random_buf just uses /dev/random, so we have only moved the goalposts, and we have not fixed this bug :(.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug