svn commit: r345380 - head/lib/libc/gen

[Bruce Evans]

On Fri, 22 Mar 2019, Bruce Evans wrote:

> On Thu, 21 Mar 2019, Conrad Meyer wrote:
>
>> Log:
>>  arc4random: Adjust example code to use uniform() API
>
> This changes the example from correct to broken.
>
> I see 4 bugs:

I see many more (old but related) bugs.

> 1. The log message says that the example uses uniform(), but it actually
>   uses arc4random_uniform().
> 2. This description still says that the example uses arc4random(), but it
> actually uses arc4random_uniform().
> 3. RAND_MAX is usually INT_MAX, so adding 1 to it gives undefined behavior.
>   The previous version carefully converted RAND_MAX to unsigned to get
>   defined behaviour.
> ..
> Oops.  Since RAND_MAX is now 2 smaller than INT_MAX, adding 1 to it doesn't
> cause overflow.  So the badness in the current example is just a style bug.
>
> 4. Another (old) bug in the example is that it says that the #define for
>   foo4random() produces a drop-in replacement for [both] rand() and 
> random().
>   The maximum value for random() is still 0x7fffffff, so the same #define
>   doesn't work for both, and it only ever worked accidentally if
>   RAND_MAX happens to be 0x7fffffff, but RAND_MAX has been 0x7ffffffd since
>   before this example existed.

5. arc4random() doesn't support seeding or returning a reproducible sequence,
    so it cannot provide a drop-in replacement for either rand() or random().

6. arc4random() is limited to 32 bits, but rand() is only limited by the
    number of bits in an int, so arc4random() cannot even provide a drop-out
    replacement for rand() on systems with more >= 34 bit ints unless
    RAND_MAX is restricted to 32 bits.

7. arc4random_uniform()'s arg is named 'upper_bound', but it is actually 1
    larger than the upper bound.

8. It is a design error for the arc4random_uniform()'s arg to be 1 larger
    than the upper bound.  This makes arc4random_uniform() harder to use
    when it works, and impossible to use when the upper bound is UINT32_MAX
    since the value 1 larger is unrepresentable them.  Portable code needs
    to use messes like:

#if RANDFOO_MAX < UINT32_MAX
#define	randfoo()	arc4random_uniform((uint32_t)RANDFOO_MAX + 1)
#else if RANDFOO_MAX == UINT32_MAX
#define	randfoo()	arc4random()
#else
#error "Can't implement drop-out replacement for randfoo() using arc4random()"
#endif

9. Casting to unsigned has another bug which I noticed while writing the
    above.  arc4random_uniform() doesn't take args of type unsigned;
    it takes args of type uint32_t.  Casting to unsigned works if
    unsigned is at least as large as uint32_t or the value is
    representable in both.  This assumption is unportable, but it became
    portable in POSIX in 2001 (?) when POSIX started requiring 32-bit
    ints.  So using unsigned is just a style bug if arc4random() is
    only supported on POSIX systems.

    Casting to either may overflow unless it is conditional on a test like
    the above.  Casting RAND_MAX to unsigned is known to work, since
    RAND_MAX <= INT_MAX.  But arc4random_uniform()'s API converts to
    uint32_t, so overflow may occur later.

Bruce