This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Add getrandom implementation [BZ #17252]
- From: Joseph Myers <joseph at codesourcery dot com>
- To: Florian Weimer <fweimer at redhat dot com>
- Cc: <libc-alpha at sourceware dot org>
- Date: Fri, 10 Jun 2016 21:30:50 +0000
- Subject: Re: [PATCH] Add getrandom implementation [BZ #17252]
- Authentication-results: sourceware.org; auth=none
- References: <20160610210303 dot 6CE3E40141175 at oldenburg dot str dot redhat 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