Hi Alejandro,
Alejandro Colomar wrote on Sat, Dec 31, 2022 at 08:08:14PM +0100:
> This makes the code much more readable and self-documented.
I object. I is a needless detour that invites confusion and bugs.
In C code, it is customary to deal with half-open intervals,
and closed intervals are rare.
For example, array indices run from 0 to sizeof(array)-1,
sizeof(array) points to the storage location beyond the last element,
and C programmers are used to that. So the was arc4random_uniform(3)
works feels familiar to C programmers, whereas your proposal
of arc4random_range(3) is idiosyncratic and provokes bugs.
> While doing this, I noticed a few bugs,
I doubt it. I think you fell into your own trap.
> * usr.bin/ssh/auth.c:
>
> - *cp = hashchars[arc4random_uniform(sizeof(hashchars) - 1)];
> + *cp = hashchars[arc4random_range(0, sizeof(hashchars) - 1)];
There is no bug here. The code goes:
const char hashchars[] = "./ABCDEFGHIJKLMNOPQRSTUVWXYZ"
"abcdefghijklmnopqrstuvwxyz0123456789"; /* from bcrypt.c */
char *cp;
/* ... */
for (cp = fake.pw_passwd + 7; *cp != '\0'; cp++)
*cp = hashchars[arc4random_uniform(sizeof(hashchars) - 1)];
sizeof(hashchars) is the size of the char array, i.e. the string length
plus one (plus one for the terminating NUL byte).
So sizeof(hashchars)-1 is the number of useful characters in the string,
which is the same as the strlen(3).
So arc4random_uniform() returns a number in the half-open
interval [0, strlen). The minimum index is 0 -> '.',
the maximum index is strlen-1 -> '9'.
This is all perfectly fine.
Your patch introduces a bug. The terminaing NUL character may now
be copied into fake.pw_passwd.
This is exactly one of the reasons why i objected to your
arc4random_range() proposal: it will cause confusion and bugs.
I think that the first hunk of your patch introduces rather than
fixes a bug makes your patch unworthy of review. It should be
summarily rejected.
Yours,
Ingo
