Re: [PATCH] Add getrandom implementation [BZ #17252]

[Joseph Myers <joseph at codesourcery dot com>]

On Fri, 10 Jun 2016, Florian Weimer wrote:

> The emulation opens /dev/random and /dev/urandom and uses
> these descriptors.  There are safeguards to detect application
> which have overridden these descriptors.

I think a substantial comment somewhere is needed explaining these 
safeguards and the rationale for them.

Are we sure we want to keep file descriptors open that the application 
can't use?  Is it not valid for applications to close all open file 
descriptors, or do you think that's only valid on startup before these 
functions have been called?

New functions need documentation in the manual, and a NEWS entry.

> +  # We use a dedicated symbol version so that we can scan the
> +  # .gnu.version_r section to identify binaries which use the
> +  # getrandom function.  This allows us to avoid lazy opening of the
> +  # emulation file descriptors if this proves too error-prone.

I understand this even less than the other safeguards.  Why is this 
different from all other functions?  What is "us"?  I don't see any 
special dynamic linker code to check for this version, for example.

> +
> +ssize_t
> +__getrandom (void *buffer, size_t length, unsigned flags)

Missing comment above function definition.  We use "unsigned int" not 
plain "unsigned".

I'm not sure quite why this code is going in posix/, or the declaration in 
unistd.h.  It's not a POSIX function, or particularly closely related to 
one.  There's an Austin Group discussion of such interfaces 
<http://austingroupbugs.net/view.php?id=859>, but no real sign that 
anything like that would end up in the next major POSIX revision, and all 
the proposal there use <stdlib.h>.

> diff --git a/posix/getrandom_emulation.c b/posix/getrandom_emulation.c

Lots more functions here missing comments.

> +static int
> +getrandom_validate_fd
> +  (const char *device, int fd, struct getrandom_emulation_fd *pfd)

That's not how we wrap prototypes.

static int
getrandom_validate_fd (const char *device, int fd,
		       struct getrandom_emulation_fd *pfd)

would be more normal style.

> +static int
> +getrandom_move_fd_target (int fd, int target)
> +{
> +  int newfd = fcntl_not_cancel (fd, F_DUPFD, target);

I'd expect F_DUPFD_CLOEXEC to be used here if available (maybe always 
available?).

> +      {
> +        int flags = O_RDONLY;
> +        if (nonblock)
> +          flags |= O_NONBLOCK;
> +        fd = open_not_cancel (device, flags, 0);

And O_CLOEXEC here.

> +#ifdef __USE_GNU
> +/* Flags for use with  getrandom.  */
> +# define GRND_NONBLOCK 1
> +# define GRND_RANDOM 2
> +
> +ssize_t __getrandom (void *__buffer, size_t __length, unsigned __flags);
> +#define getrandom(buffer, length, flags) \
> +  (0 + __getrandom (buffer, length, flags))

Missing comment on function prototype.

-- 
Joseph S. Myers
joseph@codesourcery.com