svn commit: r252608 - in head: include lib/libc/stdlib

[Bruce Evans]

On Thu, 4 Jul 2013, Andrey Chernov wrote:

> On 04.07.2013 6:47, Bruce Evans wrote:
>> Er, I think it is too dangerous to change either RAND_MAX or the offset
>> without more preparation:
>> - increasing the range returned (and increasing RAND_MAX to match) would
>>   obviously be binary-incompatible.  Old binaries may have the old RAND_MAX
>>   built in to them, but will call the new rand().  They would be broken if
>>   the new rand() returned a value larger than the old RAND_MAX.  But this
>>   change only reduces RAND_MAX.  RAND_MAX was already 1 higher than could
>>   be returned.  This change expands the range at the low end, so that 0
>>   is now returned, but returning it was always possible and should have
>>   occurred.
>
> Currently the range is reduced, not increased. In details:

That's what I said.

> Old binaries + old libc already have +1 bigger RAND_MAX and never returns 0.
> Old binaries + new libc will have +2 bigger RAND_MAX and returns 0 (0 is
> allowed by POSIX).
> The value bigger than old RAND_MAX will never returned and it even is
> impossible with the formula we use and 0 is allowed by POSIX, so I don't
> see any binary incompatibilities with that changes.

It is only because it is reduced that the change is not a complete breakage
of compatibility.

>> - changing the offset is more of a problem.  It changes the sequence
>>   generated by each fixed seed.  Of course, the sequence is not guaranteed
>>   to be repeated after all system changes.  The C90/C99 specification is
>>   actually unusably fuzzy about this.  C99 (n869.txt) says:
>
> ...
>
>> Perahps this only says that the sequence shall be repeated within each
>> "execution" of a C program, but that is not very useful.  This is not
>> fixed in POSIX, at least in old drafts.  POSIX should at least say that
>> the sequence shall be repeated across the lifetime of a single process
>> (it can be more specific about "execution").  But to be useful, the
>> repeatability must be much more than that (certainly across reboots,
>> which is already more than POSIX can explicitly specify).
>
> We already pass that moment in the past, changing old&bad formula with
> new one which cause the same effect: non-repeating sequence in the very
> global scope. We already agree that repeating depends on something like
> OS release numbers. I can't find that discussion right now.

But you are changing it in between releases.

>> Previous breakage of this is still not fixed.  Old versions used
>> /dev/random and had error handling involving use of the current
>> time when _read() failed.
>
> This specific sysctl() can't fail. You'll better to discuss this matter
> with the original change author, not with me.

Of course it can fail.  It fails whenever the kernel doesn't support the
sysctl.  This is always for old kernels.  Not good enough for a security-
related function.

>> They also spelled read() correctly,
>> so that the Standard library function rand() is not broken is
>> the application is a pure C90 one that supplies its own read().
>> The above has doesn't even have error checking, and is broken
>> if the application supplies its own sysctl().
>
> libc have a lot of places where it uses sysctl() instead of _sysctl().
> If you assume to change them all, it will be sweep change not for me,
> please ask someone else.

Not many in the C99 part of the library.  I already discussed the bug
when it was committed.

Some POSIX parts have always been broken:
- sysconf() was the primary client of sysctl(), and it always used raw
   sysctl()
- the careful namespace handling in __xuname()'s name is defeated by
   __xuname() calling sysctl().
- in libc/gen, it is mainly arc4random() that uses __sysctl(), and since
   arc4random() is not in POSIX, it can just use sysctl().

   arc4random() is also not missing the error check when it uses the
   same sysctl that is missing the error handling in sranddev() and
   sranddomdev().  arc4random() handles the error by first falling back
   to using /dev/random; if that fails, it falls back to using
   gettimeofday(), getpid(); if that fails it intentionally uses stack
   garbage.  This is what srand() and srandomdev() used to do, except
   they started at the /dev/random level and did home made stirring of
   the time bits.

   srand() and srandomdev() should do the same as arc4random() (except
   for the stirring step).  Howver, it is actually not so good to succeed
   and produce low-quality random data.  Callers have no way of knowing
   that the call didn't really work, and of course the error handling
   is not documented.

- the POSIX ctermid() used to be careful about namespaces (it just used
   _PATH_TTY), but it now uses raw sysctlbyname().
- the POSIX gethostname() was never careful about namespaces.

>> Style bug: the API name 'sranddev()' is bogus for a function that doesn't
>> used /dev/random like it used to.
>
> This is historical thing when all we have was /dev/random.

Yes, it is a bad API name (too closely tied to the implementation) that
became worse when the implementation changed.  But it may be safer to
change this than RAND_MAX :-).

Bruce