Hello Damien, On 1/1/23 00:07, Damien Miller wrote: > On Sat, 31 Dec 2022, Theo de Raadt wrote: > >> 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. > > I agree - specifying a zero upper-bound is numerically nonsensical, > and could often be the result of a bug in the caller. > > Changing it is likely to break code like this in a plausibly exploitable > way: > > elem_t *random_elem(elem_t **elems, size_t nelems) { > return elems[arc4random_uniform(nelems)]; > } The above code is already broken. In case nelems is 0, the array has exactly 0 elements, so the pointer &elems[0] is a pointer to one-past-the-last element. It is legal to hold such a pointer, but not to dereference it (I guess I don't need to quote the standard here). Such a pointer dereference *is dangerous*, and *is very-likely exploitable*. Having a random 32-bit number instead is likely to be a pointer addressing an invalid memory address, and result in a crash. And crashes are good, right? Changing the behavior of arc4random_uniform() wouldn't make this code more broken, but rather uncover the bug in it. > > Therefore IMO the only safe return from arc4random_uniform(0) is 0. I'd argue it's the opposite. 0 is the most unsafe value it can return in such a case, since it's the least likely to result in a crash. The Undefined Behavior is invoked, and in a way that is likely to modify memory that is available to the process. 42 would be a better value. An even better value would be UINT32_MAX, which would almost-certainly guarantee a crash everywhere. However, it makes more sense to just let it be an unbounded random value, which will likely result in the same crashes as with UINT32_MAX, but would be more useful for other purposes. > > That changing make it fractionally simpler to implement one particular > wrapper doesn't IMO justify it. > > -d Cheers, Alex --