Re: [PATCH v7] getrandom system call wrapper [BZ #17252]

[Florian Weimer <fweimer@redhat.com>]

On 11/17/2016 07:21 AM, Mike Frysinger wrote:
> On 14 Nov 2016 18:44, Florian Weimer wrote:
>
> just nits at this point
>
>> +/* Flags for use with getrandom.  */
>> +#define GRND_NONBLOCK 1
>> +#define GRND_RANDOM 2
>
> if they're bit flags, should we be doing 0x1/0x2 etc ?  otherwise this
> will turn into 4, 8, 16, 32, 64, etc... which gets ugly.  the kernel
> headers use hex constants.

Okay, I turned this into:

#define GRND_NONBLOCK 0x01
#define GRND_RANDOM 0x02

Do you want more zero padding?

(I'm not a fan of column alignment because it means that future patches 
will have to make whitespace-only changes to change column alignment, 
which slightly obfuscates the actual change.)

>> +/* Test getrandom with a single buffer length.  */
>> +static void
>> +test_length (char *buffer, size_t length, unsigned int flags)
>> +{
>> +  memset (buffer, 0, length);
>> +  strcpy (buffer + length, "123");
>
> while this works, it seems pointlessly fragile.  can't you treat it like
> a normal "this is the length of the buffer" and carve out space at the
> end yourself ?  i.e.

> 	memset (buffer, 0, length);
> 	static const char canary[] = "123";
> 	size_t canary_len = sizeof(canary);
> 	length -= canary_len;
> 	strcpy (buffer + length, canary);
> 	...
>
> 	ssize_t ret = getrandom (buffer, length - , flags);

I don't think this is much clearer.  I'm going to add a comment:

/* Test getrandom with a single buffer length.  NB: The passed-in
    buffer must have room for four extra bytes after the specified
    length, which are used to test that getrandom leaves those bytes
    unchanged.  */

Hopefully this is clear enough.

>
>> +  if (ret < 0)
>> +    {
>> +      if (!((flags & GRND_RANDOM)
>> +            && (flags & GRND_NONBLOCK)
>> +            && errno != EAGAIN))
>
> seems like it'd be more readable to distribute the ! and to combine the
> flags check into a single mask ?  i have to read these lines a few times
> to digest what exactly the code is trying to do.

What about this?

       /* EAGAIN is an expected error with GRND_RANDOM and
          GRND_NONBLOCK.  */
       if ((flags & GRND_RANDOM)
           && (flags & GRND_NONBLOCK)
           && errno == EAGAIN)
         return;
       printf ("error: getrandom (%zu, 0x%x): %m\n", length, flags);
       errors = true;
       return;

The second return was missing before, the old condition was actually wrong.

>> +      if (getrandom_full (buffer1, sizeof (buffer1), flags)
>> +          && getrandom_full (buffer2, sizeof (buffer2), flags))
>> +        {
>> +          if (memcmp (buffer1, buffer2, sizeof (buffer1)) == 0)
>
> maybe also add a comment that likelihood of this being the same is
> extremely rare too.

This should do it:

           /* The probability that these two 8-byte buffers are equal
              is very small (assuming that two subsequent calls to
              getrandom result are independent, uniformly distributed
              random variables).  */

>> +  for (int use_random = 0; use_random < 2; ++use_random)
>> +    for (int use_nonblock = 0; use_nonblock < 2; ++use_nonblock)
>> +      {
>> +        int flags = 0;
>
> unsigned to match the API ?

Right, thanks.

Do you have any comments about the matter of the cancellation point and 
the redirect to __libc_getrandom?

Thanks,
florian