Re: [PATCH] Improve mutt's random data generator.

"Kevin J. McCarthy" <[email protected]> Fri, 24 Apr 2026 06:54:31 +0800
Newsgroups	gmane.mail.mutt.devel
Message-ID	<aeqjJ0Y0sSF27NtR@qinghai>
On Thu, Apr 23, 2026 at 02:03:45PM +0200, Alejandro Colomar via Mutt-dev wrote:
>Hi Kevin,
>
>On 2026-04-23T14:10:31+0800, Kevin J. McCarthy wrote:
>> +/* Generate length_requested random bytes of data */
>> +void mutt_random_bytes(char *random_bytes, size_t length_requested)
>> +{
>> +#if defined(HAVE_GETRANDOM)
>> +  size_t res;
>> +
>> +  do
>> +  {
>> +    res = getrandom(random_bytes, length_requested, GRND_NONBLOCK);
>
>Do we really want to not block?  Even on ancient systems on which it
>may block, I expect one can wait a few ms (or seconds) until the entropy
>has been gathered.  That would keep the source code simpler.
>
>Or is this used frequently enough that it would slow down the program to
>unreasonable times?

I was under the impression from Greg's comments earlier that it
shouldn't block except under strange circumstances.  We use this to
generate the message boundaries and message id when sending.  It's not
time critical, but if there is something weird going on, and we have a
backup plan (the PRNG), I thought better to just fall to the backup than
wait an indeterminate amount of time.

>> +  } while ((res == (size_t) -1) && (errno == EINTR));
>
>This cast is dangerous.  It's safer to compare to literal -1.  I know
>you'll get a -Wsign-compare (part of -Wextra) diagnostic, but that's
>something that GCC should fix.  I've been using -Wno-error=sign-compare
>for some time, precisely for the false negatives.
>
>If for some reason, the type of the cast doesn't match the type of the
>variable, we'll get a bug.  On the other hand, non-casted literal -1
>will magically convert to any wider unsigned type and do the right
>thing.

I'll try removing the cast then, but it if triggers warnings and aborts 
the CI compiles on any of the sr.ht servers prefer to add it back in.

(replying to the sub-thread with Kurt lower): I realized the return type 
is actually ssize_t, but I chose to work with size_t in this case 
because of the:

     if (res < length_requested)
     {
       length_requested -= res;
       random_bytes += res;
     }

I know the first < comparison will promote to size_t.  But I was unsure 
of the behavior of adjusting the length_requested and random_bytes by a 
ssize_t, in the theoretical case that the high bit was set.  (Not that 
we'll ever make a request that big).

So I thought the cast comparing to -1 was worth the clarity trade off 
below.

But if in fact the code would be fine I'll convert to ssign_t.

Thanks Alex and Kurt!

-- 
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C  5308 ADEF 7684 8031 6BDA
signature.asc (application/pgp-signature, 833 B)