From owner-svn-src-head@FreeBSD.ORG Wed Apr 3 07:04:59 2013 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 03FF8EE4; Wed, 3 Apr 2013 07:04:59 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx07.syd.optusnet.com.au (fallbackmx07.syd.optusnet.com.au [211.29.132.9]) by mx1.freebsd.org (Postfix) with ESMTP id 8035B8DD; Wed, 3 Apr 2013 07:04:57 +0000 (UTC) Received: from mail36.syd.optusnet.com.au (mail36.syd.optusnet.com.au [211.29.133.76]) by fallbackmx07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r3374mZq031874; Wed, 3 Apr 2013 18:04:48 +1100 Received: from c211-30-173-106.carlnfd1.nsw.optusnet.com.au (c211-30-173-106.carlnfd1.nsw.optusnet.com.au [211.30.173.106]) by mail36.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r3374baV013283 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 3 Apr 2013 18:04:39 +1100 Date: Wed, 3 Apr 2013 18:04:37 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Xin LI Subject: Re: svn commit: r249035 - head/lib/libc/stdlib In-Reply-To: <201304022341.r32NfL8L096954@svn.freebsd.org> Message-ID: <20130403165736.F819@besplex.bde.org> References: <201304022341.r32NfL8L096954@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=Ov0XUFDt c=1 sm=1 a=fzJqrta_5x4A:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=ys8XlwWUekEA:10 a=B_BkEhnI1Mf147ghvr8A:9 a=CjuIK1q_8ugA:10 a=yIbjSOY98VjwACzz:21 a=abnDXnK00Rrm2gJP:21 a=TEtd8y5WR3g2ypngnwZWYw==:117 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Apr 2013 07:04:59 -0000 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