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

[Florian Weimer <fweimer at redhat dot 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