From owner-svn-src-head@freebsd.org Fri Mar 22 14:21:14 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 9B8211547A04; Fri, 22 Mar 2019 14:21:14 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id E534885667; Fri, 22 Mar 2019 14:21:13 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id AD439105DD93; Sat, 23 Mar 2019 01:21:04 +1100 (AEDT) Date: Sat, 23 Mar 2019 01:21:03 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Evans cc: Conrad Meyer , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r345380 - head/lib/libc/gen In-Reply-To: <20190322050731.V3169@besplex.bde.org> Message-ID: <20190323005050.Y2389@besplex.bde.org> References: <201903211445.x2LEj8U0002125@repo.freebsd.org> <20190322050731.V3169@besplex.bde.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.2 cv=UJetJGXy c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=5Q5Ff2_q-dyTL55U5OwA:9 a=CjuIK1q_8ugA:10 X-Rspamd-Queue-Id: E534885667 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.96 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.96)[-0.964,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 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: Fri, 22 Mar 2019 14:21:14 -0000 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