Bug 146473 – Errors in read() are not handled in WTF::cryptographicallyRandomValuesFromOS.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Keith Miller]

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.

[Keith Miller]

Created attachment 255857 [details]
Patch

[Keith Miller]

Created attachment 255860 [details]
Patch

Changed the name of the crashing functions to make it clear they were related to /dev/urandom

[Keith Miller]

rdar://problem/21552607

[Filip Pizlo]

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 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]

Created attachment 255864 [details]
Patch

Fixed issue with currentRead being < 0

[Michael Saboff]

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 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]

Created attachment 255868 [details]
Patch

Removed checks for EINTR and EAGAIN from the open call.

[Keith Miller]

Created attachment 255869 [details]
Patch

For some reason the other patch didn't update.

[Geoffrey Garen]

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 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]

> 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]

Created attachment 255872 [details]
Patch

Refactored code slightly.

[WebKit Commit Bot]

Comment on attachment 255872 [details]
Patch

Clearing flags on attachment: 255872

Committed r186151: <http://trac.webkit.org/changeset/186151>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

<rdar://problem/21939126>

[Geoffrey Garen]

LOL. arc4random_buf just uses /dev/random, so we have only moved the goalposts, and we have not fixed this bug :(.