Hi Alexandro,
i fail to see the point. We do not usually add extra functions
if the same effect can be be attained with one-liners. There is
significant value in keeping the API as small as possible, it
makes the API easier to learn and it makes programming mistakes
less likely. On top of that, code using the one-liner is usually
easier to read than code involving yet another wrapper.
On top of that, consider that arc4random_uniform(upper_bound)
produces a number in [0, upper_bound), *not* in [0, upper_bound].
Consistency of an API is similarly important as smallness.
So introducing a variant that produces [min, max] rather than
[min, max) looks like a non-starter to me. You are setting a
giant trap provoking off-by-one errors!
Some additional comments follow in-line, even though i doubt they
matter much: i think that nothing should be added, and you did not
provide any rationale why this might be needed.
Alejandro Colomar wrote on Sat, Dec 31, 2022 at 07:18:55PM +0100:
> diff --git a/include/stdlib.h b/include/stdlib.h
> index ab8a2ae90c3..16b7dc43afc 100644
> --- a/include/stdlib.h
> +++ b/include/stdlib.h
> @@ -313,6 +313,7 @@ u_quad_t strtouq(const char *__restrict, char **__restrict, int);
>
> uint32_t arc4random(void);
> uint32_t arc4random_uniform(uint32_t);
> +uint32_t arc4random_uniform(uint32_t, uint32_t);
Uh oh. That looks doubleplusungood.
> --- a/lib/libc/crypt/arc4random.3
> +++ b/lib/libc/crypt/arc4random.3
> @@ -46,6 +46,8 @@
[...]
> @@ -95,16 +97,47 @@
> In the worst case, this function may consume multiple iterations
> to ensure uniformity; see the source code to understand the problem
> and solution.
> +.Pp
> +.Fn arc4random_range
> +is similar to
> +.Fn arc4random_uniform ,
I very strongly object to this description. I is an outright lie!
arc4random_uniform -> upper_bound) *exclusive*
arc4random_range -> upper_bound] *inclusive*
> +and will return a single 32-bit value,
The word "will" is unwelcome in manual pages, unless you have
some exceptional justification why it is needed in a particular case.
> +uniformly distributed,
> +within the inclusive range
> +.Pf [ Fa min ,
> +.Fa max ].
> +If the arguments are reversed,
> +that is,
> +if
> +.Fa max
> +<
> +.Fa min ,
> +it will return a single 32-bit value,
> +uniformly distributed,
> +outside of the exclusive range
> +.Pf ( Fa max ,
> +.Fa min ).
Is this ever needed in practice?
It sounds complicated and confusing.
We should not add API specifications just because we can,
espially not in libc.
> .Sh RETURN VALUES
> These functions are always successful, and no return value is
> reserved to indicate an error.
> +.Sh CAVEATS
> +.Fn arc4random_range
> +doesn't produce correct output when
> +.Fa max
> +==
> +.Fa min
> +- 1.
Looks like a landmark symptom of bad API design: setting a trap for
the unwary with no good reason. We should not provide bad API
functions and then document their shortcomings.
> .Sh SEE ALSO
> .Xr rand 3 ,
> .Xr rand48 3 ,
> .Xr random 3
> .Sh HISTORY
> These functions first appeared in
> -.Ox 2.1 .
> +.Ox 2.1 ,
> +except
> +.Fn arc4random_range ,
> +which appeared in
> +.Ox XXX .
> .Pp
> The original version of this random number generator used the
> RC4 (also known as ARC4) algorithm.
> diff --git a/lib/libc/crypt/arc4random_uniform.c b/lib/libc/crypt/arc4random_uniform.c
> index a18b5b12381..40957910b96 100644
> --- a/lib/libc/crypt/arc4random_uniform.c
> +++ b/lib/libc/crypt/arc4random_uniform.c
> @@ -2,6 +2,7 @@
>
> /*
> * Copyright (c) 2008, Damien Miller <djm@openbsd.org>
> + * Copyright (c) 2022, Alejandro Colomar <alx@kernel.org>
Adding a Copyright notice is inappropriate in most parts of OpenBSD
except for the main authors of the file. Merely adding something
that is Copyright-worthy is not enough. Besides, i suspect your
patch reaches the Copyright threshold only due to the comment -
the function itself is likely trivial even according to the low
threshold of Copyright law.
Yours,
Ingo
> *
> * Permission to use, copy, modify, and distribute this software for any
> * purpose with or without fee is hereby granted, provided that the above
> @@ -55,3 +56,14 @@ arc4random_uniform(uint32_t upper_bound)
> return r % upper_bound;
> }
> DEF_WEAK(arc4random_uniform);
> +
> +/*
> + * Calculate a uniformly-distributed random number in the range [min, max],
> + * avoiding bias.
> + */
> +uint32_t
> +arc4random_range(uint32_t min, uint32_t max)
> +{
> + return arc4random_uniform(max - min + 1) + min;
> +}
> +DEF_WEAK(arc4random_range);
[...]
No comments:
Post a Comment