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

Alejandro Colomar via Mutt-dev <[email protected]> Thu, 23 Apr 2026 14:03:45 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<aeoHLd4lTaVRsljs@devuan>
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?

> +  } 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.

See <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119011>.

> +
> +  if ((res == (size_t) -1) || (res < length_requested))

Same thing about the cast.

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

Maybe prng_random_bytes() should get a pointer to the end of the array
instead of a length, so that it doesn't need to be updated?

The idea is similar to Plan9's strecpy(2) string-copying function,
documented here: <http://man.cat-v.org/plan_9_3rd_ed/2/strcat>


Other than these minor comments, the patch seems reasonable to me.


Have a lovely day!
Alex

> +    }
> +    prng_random_bytes(random_bytes, length_requested);
> +  }
> +#elif defined(HAVE_ARC4RANDOM_BUF)
> +  arc4random_buf(random_bytes, length_requested);
> +#else
> +  prng_random_bytes(random_bytes, length_requested);
> +#endif
> +}
>  
>  /* Generate and Base64 encode 96 random bits and fill the buffer
>     output_B64 with the result. */
> diff --git a/mutt_random.h b/mutt_random.h
> index ed9fdf29..0417e0d6 100644
> --- a/mutt_random.h
> +++ b/mutt_random.h
> @@ -25,6 +25,5 @@ typedef union random64
>  } RANDOM64;
>  
>  void mutt_base64_random96(char output_B64[static 17]);
> -void mutt_random_bytes(char *random_bytes, int length_requested);
> -void mutt_reseed(void);
> +void mutt_random_bytes(char *random_bytes, size_t length_requested);
>  #endif
> -- 
> 2.53.0
> 

-- 
<https://www.alejandro-colomar.es>
signature.asc (application/pgp-signature, 833 B)