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 7F6CF3858D1E for ; Sat, 31 Dec 2022 08:51:33 +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:from; d=openbsd.org; b=LJdlINLIUlaqQZgxJKWNNkwNsDsZpS69USET cjSAgqeM/MyYkPauGBGfUQmtHZucd5gFKLqdoBNM8e+O17LNe9tj9A7SmXZHvsjLK+s2ig SnGglBspf1dpOWHpyT5ovJQmk2NM9nWTMmDgk27o195fzyXS6uf7p9IC4SuvZZvKpWJm06 pAZi5Utt8CchKp7XX6IQSn9WO/M5RvlphQjJ8z144ajQJfIRsdAU7Y1Hui+AAs3sD10hjs fr+22UmX5AAf022WSm49K+2x5H7hJ7zruZlSiB+tvEtq7g4BiM4NKZ7StzfH1TZHx8j4Ii +dSoWBtPnFm0VuHN7++qsVieuA== Received: from cvs.openbsd.org (localhost [127.0.0.1]) by cvs.openbsd.org (OpenSMTPD) with ESMTP id f17795e7; Sat, 31 Dec 2022 01:51:32 -0700 (MST) From: "Theo de Raadt" cc: Alejandro Colomar , otto@cvs.openbsd.org, djm@cvs.openbsd.org, libc-alpha@sourceware.org, Alejandro Colomar , Theo de Raadt , "Todd C . Miller" , "Jason A. Donenfeld" , =?us-ascii?Q?=3D=3Fus-ascii=3FQ=3F=3D3D=3D3FUTF-8=3D3Fq=3D3FCristian?= =?us-ascii?Q?=3D3D20Rodr=3D3DC3=3D3DADguez=3D3F=3D3D=3F=3D?= , Adhemerval Zanella , Yann Droneaud , Joseph Myers , otto@cvs.openbsd.org, djm@cvs.openbsd.org Subject: Re: [PATCH] Give a useful meaning to arc4random_uniform(0); In-reply-to: <5084.1672476619@cvs.openbsd.org> References: <20221231023653.41877-1-alx@kernel.org> <5084.1672476619@cvs.openbsd.org> Comments: In-reply-to "Theo de Raadt" message dated "Sat, 31 Dec 2022 01:50:19 -0700." MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Sat, 31 Dec 2022 01:51:32 -0700 Message-ID: <78070.1672476692@cvs.openbsd.org> X-Spam-Status: No, score=-9.3 required=5.0 tests=BAYES_00,DKIM_INVALID,DKIM_SIGNED,GIT_PATCH_0,KAM_DMARC_STATUS,MISSING_HEADERS,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