svn commit: r249035 - head/lib/libc/stdlib

[Bruce Evans]

On Tue, 2 Apr 2013, Xin LI wrote:

> Log:
>  Replace access to /dev/random with the kernel pseudo-random number
>  source sysctl(KERN_ARND) and remove the fallback code.
>
>  Obtained from:	OpenBSD
>  Reviewed by:	secteam

Really?

> Modified: head/lib/libc/stdlib/rand.3
> ==============================================================================
> --- head/lib/libc/stdlib/rand.3	Tue Apr  2 21:34:38 2013	(r249034)
> +++ head/lib/libc/stdlib/rand.3	Tue Apr  2 23:41:20 2013	(r249035)
> @@ -32,7 +32,7 @@
> .\"     @(#)rand.3	8.1 (Berkeley) 6/4/93
> .\" $FreeBSD$
> .\"
> -.Dd September 4, 2012
> +.Dd April 2, 2013
> .Dt RAND 3
> .Os
> .Sh NAME
> @@ -91,9 +91,7 @@ seeded with a value of 1.
> .Pp
> The
> .Fn sranddev
> -function initializes a seed using the
> -.Xr random 4
> -random number device which returns good random numbers.
> +function initializes a seed using pseudo-random numbers obtained from the kernel.

It no longer claims to return good random numbers.

Better not give implementation details.

Lexical style bug: line longer than 80 characters.

> Modified: head/lib/libc/stdlib/rand.c
> ==============================================================================
> --- head/lib/libc/stdlib/rand.c	Tue Apr  2 21:34:38 2013	(r249034)
> +++ head/lib/libc/stdlib/rand.c	Tue Apr  2 23:41:20 2013	(r249035)
> ...
> @@ -112,28 +111,20 @@ u_int seed;
>  * sranddev:
>  *
>  * Many programs choose the seed value in a totally predictable manner.
> - * This often causes problems.  We seed the generator using the much more
> - * secure random(4) interface.
> + * This often causes problems.  We seed the generator using pseudo-random
> + * data from the kernel.
>  */
> void
> sranddev()
> {
> -	int fd, done;
> +	int mib[2];
> +	size_t len;
>
> -	done = 0;
> -	fd = _open("/dev/random", O_RDONLY | O_CLOEXEC, 0);
> -	if (fd >= 0) {
> -		if (_read(fd, (void *) &next, sizeof(next)) == sizeof(next))
> -			done = 1;
> -		_close(fd);
> -	}
> -
> -	if (!done) {
> -		struct timeval tv;
> -
> -		gettimeofday(&tv, NULL);
> -		srand((getpid() << 16) ^ tv.tv_sec ^ tv.tv_usec);
> -	}

_open() and _read() are unlikely to fail, but there was error checking and
handling for them.  There was no error checking for the gettimeofday()
call in the error handling.  The man page's documentaion of the
implementation details was wrong if the error handling was used.

This is part of the implementation of the STANDARD library, so it
cannot use POSIX extensions directly.  It was careful about this for
_open() and _close(), but not for gettimeofday() or getpid().  This
is completely backwards.  _open and _read are in namespace.h so there
is no need to spell them with an underscore, while gettimeofday and
getpid is not in namespace.h so they do need to be spelled with an
underscore.

This file has massive other (link-time) namespace pollution which makes
the above namespace errors moot.  The STANDARD functions that it
implements are rand() and srand().  It implements the extensions
rand_r() and sranddev().  These are not hidden in any way, so they
break public symbols of the same name in the application namespace.
They are not used by rand() or srand(), so they don't break calls to
these functions.  They should break linkage to the standard function
if the application has public symbols of the same name.  Such breakage
is good for detecting the error, but it only works with static linkage.

> +	len = sizeof(next);
> +
> +	mib[0] = CTL_KERN;
> +	mib[1] = KERN_ARND;
> +	sysctl(mib, 2, (void *)&next, &len, NULL, 0);
> }

The sysctl() is certain to fail on old kernels (like open of /dev/random
on even older kernels), but there is no longer any error checking or
handling.  The contents of `next' on error is indeterminate (not documented
in the man page), but is probably unchanged.  Applications can actually
detect this error although though the API doesn't support this, by
using the documented implementation details and assuming that errno
is properly left changed if the syscall fails (set errno to 0 before
the call here and check it after).

sysctl() is not even in any version POSIX.1, so it is further from being
directly usable than _open() and _read().  It is like gettimeofday()
above -- not in namespace.h.

Style bugs:
- blank line that separates the initialization of `len' from its use
- unsorted initializations (`len' is a secondary part of the sysctl data
   so it might as well be initialized after 'mib'
- use of sysctl() instead of sysctlbyname()
- existence of KERN_ARND so that use of sysctl() is possible.  KERN_ARND
   is much newer than sysctlbyname(), so it should have been OID_AUTO.
   Maybe it exists for compatibility with other OS's, but it shouldn't be
   used in new code in FreeBSD.
- bogus cast of &next.  In the old code, the cast had an additional style
   bug (space after it), but it was necessary and probably sufficient for
   supporting K&R with no prototypes.  Now it is certainly insufficient, since
   the NULL and 0 args to sysctl() are not cast.

After changing to use sysctlbyname(), most of the other style bugs go
away automatically, and the code becomes cleaner, but restoring the error
handling would make it messy again.

> Modified: head/lib/libc/stdlib/random.c
> ==============================================================================
> --- head/lib/libc/stdlib/random.c	Tue Apr  2 21:34:38 2013	(r249034)
> +++ head/lib/libc/stdlib/random.c	Tue Apr  2 23:41:20 2013	(r249035)
> void
> srandomdev(void)
> {
> -	int fd, done;
> +	int mib[2];
> 	size_t len;
>
> 	if (rand_type == TYPE_0)
> -		len = sizeof state[0];
> +		len = sizeof(state[0]);
> 	else
> -		len = rand_deg * sizeof state[0];
> -
> -	done = 0;
> -	fd = _open("/dev/random", O_RDONLY | O_CLOEXEC, 0);
> -	if (fd >= 0) {
> -		if (_read(fd, (void *) state, len) == (ssize_t) len)
> -			done = 1;
> -		_close(fd);
> -	}
> +		len = rand_deg * sizeof(state[0]);
>
> -	if (!done) {
> -		struct timeval tv;
> -
> -		gettimeofday(&tv, NULL);
> -		srandom((getpid() << 16) ^ tv.tv_sec ^ tv.tv_usec);
> -		return;
> -	}
> +	mib[0] = CTL_KERN;
> +	mib[1] = KERN_ARND;
> +	sysctl(mib, 2, state, &len, NULL, 0);
>
> 	if (rand_type != TYPE_0) {
> 		fptr = &state[rand_sep];

Now the function is a BSD extension, so there are no namespace problems.

There are the same error handling problems as above.

The style bug of casting `state' is fixed.  It was a larger style bug in
the old version, since the function definition isn't K&R.

Bruce