Re: [PATCH] Give a useful meaning to arc4random_uniform(0);

[Alejandro Colomar <alx.manpages@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4270 bytes --]

Hello Ariadne,

On 1/1/23 08:48, Ariadne Conill wrote:
> On Sun, 1 Jan 2023, Alejandro Colomar via Libc-alpha wrote:
>> On 1/1/23 00:07, Damien Miller wrote:
>> 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?
> 
> In scenarios where available address space is constrained, the likelihood of a 
> crash verses state corruption elsewhere in a program is reduced considerably.  I 
> think we should avoid defining interfaces based on the assumption that some 
> minimal amount of address space is available.
> 
>> Changing the behavior of arc4random_uniform() wouldn't make this code more 
>> broken, but rather uncover the bug in it.
> 
> The better approach would be for random_elem to check that there is at least one 
> element available.

Agree.

>  Making arc4random_uniform(0) do something unexpected will 
> probably break legitimate code.  I can think of many situations where 
> arc4random_uniform(0) would be legitimately called.

Please show (and/or link to) some examples, if possible.  I'm really curious.

> 
>>> 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.
> 
> If you are using arc4random_uniform to blindly pick elements out of an array 
> without doing the bounds checking yourself, you're already setting yourself up 
> for failure.  In an ideal world, we would add an additional API which is 
> designed for picking elements and which did this type of bounds check and then 
> failed safely.  Something like:
> 
> inline void *
> arc4random_pickelem(void **elems, size_t elemsize, size_t nelems)
> {
>      if (__builtin_object_size(elems, 0) < (nelems * elemsize)) {
>          errno = EINVAL;
>          return NULL;
>      }
> 
>      ptrdiff_t diff = arc4random_uniform(nelems) * elemsize;
>      return (char *)(elems + diff);
> }
> 
> Changing arc4random_uniform(0) to return non-zero will probably break monte 
> carlo simulations and such which reasonably depend on the behavior that 
> arc4random_uniform(0) == 0.

I don't know that code.  Please show it or link to it.

> 
>>
>> 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.
> 
> What purpose do you envision where arc4random_uniform(0) being non-zero would be 
> considered useful?  If you want to safely pick elements from arrays at random, 
> then we should build an interface for doing this safely, rather then changing 
> the behavior of pre-existing ones.

So far, the only code I've touched myself that needed this is the _range() 
variant that I wrote for selecting a value within a [min, max] range.  I don't 
even need to modify arc4random_uniform(3) for this, because it's not implemented 
in terms of arc4random_uniform(3), but rather in terms of 
arc4random(3)/getrandom(2), and I had to implement the _uniform() API myself. 
Since I didn't look at any existing implementation, I wrote the behavior for (0) 
that was most relevant for my use case, which is to allow the _range() function 
to work well for any input.

I will probably rename my _uniform() implementation to _uniform_wrap(), to avoid 
confusing users.

The manual page doesn't help either, since it doesn't document what happens on 
(0).  Maybe that's something you could add to CAVEATS in you manual page.

You can see the code here:
<https://github.com/shadow-maint/shadow/pull/624/files>

Maybe Jason has more examples to show you from Linux internals, since he 
implemented the same behavior for Linux recently.

> 
> Ariadne

Cheers,
Alex

-- 
<http://www.alejandro-colomar.es/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]