[PATCH] [libc++] Don't return uninitialized data from random_device::operator()

[David Majnemer]

Committed as r210061.


On Mon, Jun 2, 2014 at 8:58 AM, Marshall Clow <mclow.lists at gmail.com> wrote:

> On May 28, 2014, at 10:27 PM, David Majnemer <david.majnemer at gmail.com>
> wrote:
>
> Updated to address review comments.
>
>
> LGTM - thanks!
>
> — Marshall
>
>
>
> On Wed, May 28, 2014 at 7:23 AM, Marshall Clow <mclow.lists at gmail.com>
> wrote:
>
>>
>> On May 27, 2014, at 11:44 AM, David Majnemer <david.majnemer at gmail.com>
>> wrote:
>>
>> On Tue, May 27, 2014 at 7:45 AM, Marshall Clow <mclow.lists at gmail.com>
>>>  wrote:
>>>
>>>>
>>>> On May 20, 2014, at 2:25 AM, David Majnemer <david.majnemer at gmail.com>
>>>> wrote:
>>>>
>>>> Oops, sent out the wrong version of this patch.  Attached is what I
>>>> intended to send.
>>>>
>>>>
>>>> On Tue, May 20, 2014 at 12:28 AM, David Majnemer <
>>>> david.majnemer at gmail.com> wrote:
>>>>
>>>>> random_device::operator() as currently implemented does not correctly
>>>>> handle errors returned by read.  This can result in it returning
>>>>> uninitialized data.
>>>>>
>>>>> To fix this, wrap the call to read in a loop.
>>>>>
>>>>
>>>> I like this; but can you think of any way to test it?
>>>>
>>>
>> I've added a test for the EOF case.
>>
>> I have written a test for EINTR case but I did not included it because
>> it's inherently not reliable. It checks the output of operator() to see if
>> signals resulted in us getting a lot of zero return results.
>>
>>
>> I get a couple of signedness warnings when building:
>>
>> + for FILE in '../src/*.cpp'
>> + /Sources/LLVM/bin/bin/clang++ -c -g -Os -arch i386 -arch x86_64
>> -nostdinc++ -std=c++11 -fstrict-aliasing -Wall -Wextra -Wshadow
>> -Wconversion -Wpadded -Wstrict-aliasing=2 -Wstrict-overflow=4 -I../include
>> ../src/random.cpp
>> ../src/random.cpp:78:14: warning: implicit conversion changes signedness:
>> 'ssize_t' (aka 'long') to 'unsigned long' [-Wsign-conversion]
>>         i += s;
>>           ~~ ^
>> 1 warning generated.
>> ../src/random.cpp:78:14: warning: implicit conversion changes signedness:
>> 'ssize_t' (aka 'long') to 'unsigned long' [-Wsign-conversion]
>>         i += s;
>>           ~~ ^
>> 1 warning generated.
>>
>> I get that read can only return -1, 0, and positive numbers, and you’re
>> already checking for -1 and 0, so I think that you can safely cast s to a
>> size_t before doing the addition.
>>
>>
>> Also, from a readability standpoint, wouldn’t a while loop be better than
>> the for loop?
>> Something like this:
>>
>>     while (n > 0)
>>     {
>>         ssize_t s = read(__f_, p, n);
>>         if (s == 0)
>>             __throw_system_error(ENODATA, "random_device got EOF");
>>         if (s == -1)
>>         {
>>             if (errno != EINTR)
>>                 __throw_system_error(errno, "random_device got an
>> unexpected error");
>>             continue;
>>         }
>>         i -= (size_t) s;
>>         p += (size_t) s;
>>     }
>>
>>
>>
> <t.patch>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140602/ba8c510a/attachment.html>