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

[Jilles Tjoelker]

On Wed, Apr 03, 2013 at 06:04:37PM +1100, Bruce Evans wrote:
> > 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.

In some sense, omitting the error handling wires down that capability
mode must continue to permit KERN_ARND.

> 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.

The purpose of namespace.h and un-namespace.h is to declare the
underscore versions using the normal headers. Therefore, all functions
in namespace.h must be called from libc as their underscore versions.

The functions in namespace.h are basically all functions that have or
had to be overridden by a threading library, such as for cancellation
points, avoiding interference with SIGTHR and libc_r's file descriptor
trickery.

> 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.

With dynamic linking there is no error at all for functions defined by
the main executable, as long as libc itself does not call the functions
by their unadorned names. For example, if the application defines a
public symbol rand_r, it completely hides libc's version. If the symbol
is in a library belonging to the application, ld may be confused about
which version to use.

> > +	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.

It may indeed be useful to add more to namespace.h to avoid interference
from the application. Where the functions need not be interposed or used
by libthr, they should be removed from Symbol.map so they are called
directly instead of via the PLT in libc.so. The fact that libthr is a
separate library makes the latter part harder.

> 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.

I think it is fine to have static OIDs for sysctls that are very
commonly used because it saves a system call to translate the name to
the OID.

> - 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.

The cast is indeed against style.

-- 
Jilles Tjoelker