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

[Kevin J. McCarthy]

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.

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





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

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: 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.

signature.asc
Description: PGP signature