From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from cvs.openbsd.org (cvs.openbsd.org [199.185.137.3]) by sourceware.org (Postfix) with ESMTPS id 41D013858D1E for ; Sat, 31 Dec 2022 08:50:22 +0000 (GMT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; s=selector1; bh=HRZu1PGq0G v+RoHf4d5KU817TdiS1EX6L7a6gLBOxOE=; h=date:references:in-reply-to: subject:cc:cc:to:from; d=openbsd.org; b=tro0kkL8InTM0MIHdfigM6wdLwAOsb NdYUyza7JDt4dJQVT944rbI9cxImPmSXkqZ8IhThZ6PKwiNda/qSLUVdcJEqhwidY9yNrT 9XcZEJbfobnqsAIZtaOw9ijrOqlbbR2z4mpevWO9Ba6xJvtdRIWBQPmOte4sPZgTRZS3fX fsImQmo0v53jbyNCG3tnCYufqjN0SwUWZN3w9PxUCfjKL8dZZ8ya5o0rL+33X3UzAZj3Q0 rUUqlDB1bq/O/sxwMVopNyDIkSsoQJCUoZokqnDXp2+3+q7l8jdvlGZOuU76b1M/JOGUgt JJBWXz0anCRJ8W5M4CbOh77vCkMGFN8A== Received: from cvs.openbsd.org (localhost [127.0.0.1]) by cvs.openbsd.org (OpenSMTPD) with ESMTP id bb3e5c62; Sat, 31 Dec 2022 01:50:19 -0700 (MST) From: "Theo de Raadt" To: Alejandro Colomar cc: libc-alpha@sourceware.org, Alejandro Colomar , Theo de Raadt , "Todd C . Miller" , "Jason A. Donenfeld" , =?us-ascii?Q?=3D=3FUTF-8=3Fq=3FCristian=3D20Rodr=3DC3=3DADguez=3F=3D?= , Adhemerval Zanella , Yann Droneaud , Joseph Myers cc: otto@cvs.openbsd.org, djm@cvs.openbsd.org Subject: Re: [PATCH] Give a useful meaning to arc4random_uniform(0); In-reply-to: <20221231023653.41877-1-alx@kernel.org> References: <20221231023653.41877-1-alx@kernel.org> Comments: In-reply-to Alejandro Colomar message dated "Sat, 31 Dec 2022 03:36:54 +0100." MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Sat, 31 Dec 2022 01:50:19 -0700 Message-ID: <5084.1672476619@cvs.openbsd.org> X-Spam-Status: No, score=-9.0 required=5.0 tests=BAYES_00,DKIM_INVALID,DKIM_SIGNED,GIT_PATCH_0,KAM_DMARC_STATUS,RCVD_IN_DNSWL_LOW,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Your proposal failed to update the manual page. That was my first hint that somethign is amiss. Right now the manual it is super clear that it returns a value "less than upper_bound". Which by prototype is uint32_t. How would you rewrite the manual page to explain this behaviour? Since your change choose a strange condition where it will return a value greater than upper_bound. Also, right now an (incorrect?) call of arc4random_uniform(0) will return 0, but with your proposal it will return a non-zero number. Have you audited the entire universe of software to ensure that your change doesn't introduce a bug in some other piece of software? I doubt you did that. Very unprofessional of you to not study the impact and just wave the issue away. I think Special-casing the value of 0 to mean something new and undocumented behaviour makes no sense. It is even potentially undocumentable. The decision to return 0 was made very carefully. It results programs passing mathematically calculated erroneous values not going further off the rails in a compliant fashion. If they are using the value to index into an array, they will tend to access in-range, rather than out of range. Do you like introducing memory safety bugs into software you did not review but which will be affected by your proposal to change a standard well-known API which has been in use since 2008. I do not like your proposal at all. A function like arc4random_range() is even more likely to be used wrong by passing backwards points and you seem to have a lot of hubris to add a range check to it. Is this really all just "everyone else's big code will be perfect like my 12 lines of code"? Also, you have not cc'd all authors of this layer. I am adding them.=20=20 Alejandro Colomar wrote: > Special-casing it in the implementation to return 0 was useless. > Instead, considering 0 as the value after UINT32_MAX has the property > that it allows implementing the following function without any > special cases: >=20 > uint32_t > arc4random_range(uint32_t min, uint32_t max) > { > return arc4random_uniform(max - min + 1) + min; > } >=20 > This works for any values of min and max (as long as min <=3D max, of > course), even for (0, UINT32_MAX). >=20 > Oh, and the implementation of arc4random_uniform(3) is now 2 lines > simpler. :) >=20 > This will work with the current implementation because powerof2(3) will > also consider 0 as a power of 2. See powerof2(3): >=20 > SYNOPSIS > #include >=20 > int powerof2(x); >=20 > DESCRIPTION > This macro returns true if x is a power of 2, and false > otherwise. >=20 > 0 is considered a power of 2. This can make sense con=E2=80=90 > sidering wrapping of unsigned integers, and has interest=E2=80=90 > ing properties. >=20 > Cc: Theo de Raadt > Cc: Todd C. Miller > Cc: "Jason A. Donenfeld" > Cc: Cristian Rodr=C3=ADguez > Cc: Adhemerval Zanella > Cc: Yann Droneaud > Cc: Joseph Myers > Signed-off-by: Alejandro Colomar > --- >=20 > Hi, >=20 > I CCd Theo and Todd, because theirs is the original implementation, and > while this is a useful feature (IMO), it wouldn't make sense to do it > without consensus with other implementations, and especially with the > original implementation. >=20 > I found this useful for shadow, where the existing code had a function > that produced a "random" value within a range, but due to the bogus > implementation, it had bias for higher values. Implementing a *_range() > variant in terms of *_uniform() made it really simple, but the > *_uniform() function needed to do something useful for 0 for that to > work. >=20 > Cheers, >=20 > Alex >=20 > stdlib/arc4random_uniform.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) >=20 > diff --git a/stdlib/arc4random_uniform.c b/stdlib/arc4random_uniform.c > index 5aa98d1c13..1cd52c0d1c 100644 > --- a/stdlib/arc4random_uniform.c > +++ b/stdlib/arc4random_uniform.c > @@ -29,15 +29,13 @@ > the asked range after range adjustment. >=20=20 > The algorithm avoids modulo and divide operations, which might be cos= tly > - depending on the architecture. */ > + depending on the architecture. > + > + 0 is treated as if it were UINT32_MAX + 1, and so arc4random_uniform(= 0) > + is equivalent to arc4random(). */ > uint32_t > __arc4random_uniform (uint32_t n) > { > - if (n <=3D 1) > - /* There is no valid return value for a zero limit, and 0 is the > - only possible result for limit 1. */ > - return 0; > - > /* Powers of two are easy. */ > if (powerof2 (n)) > return __arc4random () & (n - 1); > --=20 > 2.39.0 >=20