Date: Sat, 23 Mar 2019 01:21:03 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Evans <brde@optusnet.com.au> Cc: Conrad Meyer <cem@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r345380 - head/lib/libc/gen Message-ID: <20190323005050.Y2389@besplex.bde.org> In-Reply-To: <20190322050731.V3169@besplex.bde.org> References: <201903211445.x2LEj8U0002125@repo.freebsd.org> <20190322050731.V3169@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190323005050.Y2389>