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