Hi Kevin,
On 2026-04-24T06:54:31+0800, Kevin J. McCarthy wrote:
> 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.
Yup; today, it shouldn't block except under very strange circumstances.
However, ancient Linux systems could block under normal circumstances.
(I don't know how many years ago that was.)
> 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.
In ancient systems, where getrandom(2) would block under normal
circumstances, I think it's fine to be a bit slow. We don't need to be
very fast generating message boundaries and message IDs, especially on
ancient systems.
And on new systems, getrandom(2) does not block except on very weird
situations *at boot time*. It can't block after boot at all. Since
mutt(1) is not going to be used at boot time, it's not worth the extra
complexity.
I would prefer not passing NONBLOCK at all. Not failing and not adding
extra code.
>
> > > + } 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.
Ok. (Alternatively, consider -Wno-error=sign-compare.)
> (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).
On systems where sizeof(ptrdiff_t) == sizeof(size_t), which is pretty
much all systems we care about --AFAIK--, having the high bit set would
mean requesting more bytes than PTRDIFF_MAX, and thus requesting more
bytes than the maximum possible size of an object.
Defining an object of that size would already be undefined behavior.
Of course, that's not possible in the stack, and neither in .bss. The
only way to attempt to get such an object is through malloc(3), which
of course will fail with ENOMEM if you try to do that, precisely to not
trigger UB.
And if you somehow trick the system to actually give you an object of
more than PTRDIFF_MAX (or equivalently, SSIZE_MAX) bytes, then we'd have
UB already, because the standard doesn't allow that. glibc itself would
probably misbehave when handling the object, because some glibc internal
code will miserably fail for such an object.
>
> 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.
You may also need to update the type of 'length_requested', to prevent
mixing signed and unsigned types, which will rightfully give warnings.
I think it may be easier to just remove the casts.
> Thanks Alex and Kurt!
You're welcome!
Have a lovely night!
Alex
>
> --
> Kevin J. McCarthy
> GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
--
<https://www.alejandro-colomar.es>
signature.asc
Description: PGP signature
