Hi Kevin,
On 2026-07-07T07:01:38+0800, Kevin J. McCarthy wrote:
[...]
> > > +/* Compare 2 creation dates and the addresses. For sorting. */
> > > +static int _crypt_compare_expires(const void *a, const void *b)
> > > +{
> > > + const crypt_key_t * const *s = (const crypt_key_t * const *) a;
> > > + const crypt_key_t * const *t = (const crypt_key_t * const *) b;
> > > + unsigned long ts = 0, tt = 0;
> >
> > Why don't we use time_t? Can it overflow?
>
> The field according to the GPGME doc is a unsigned long:
>
> https://www.gnupg.org/documentation/manuals/gpgme/Key-objects.html
>
> unsigned long int timestamp
Ahhh, that explains it; thanks!
> This is the creation timestamp of the subkey. This is (unsigned
> long)(-1) if the timestamp is invalid, and 0 if it is not available. Note
> that an invalid timestamp indicates a bug in the engine.
>
> unsigned long int expires
>
> This is the expiration timestamp of the subkey, or 0 if the subkey does
> not expire.
>
[...]
> > > + strncpy(tstr, p, 11);
> > > + tstr[4] = '\0';
> > > + tstr[7] = '\0';
> >
> > strncpy(3) should never be used for copying strings with truncation.
> >
> > Some of its issues are:
> >
> > - It forces you to terminate explicitly, which is error-prone.
> > - It doesn't detect truncation.
> > - It makes it more difficult to know if the zeroing is superfluous or
> > necessary.
>
> I agree. As you noticed in this case, I was moving very old code around.
> We can certainly rewrite it, but I'd prefer to do that in a separate commit.
Yup; certainly.
> > In this case, I'd use the following to replace the code above:
> >
> > const char *y, *m, *d;
> >
> > if (strlen(p) >= sizeof(tstr))
> > goto bail;
> > strcpy(tstr, p);
> > p = tstr;
> > y = strsep(&p, "-");
> > m = strsep(&p, "-");
> > d = strsep(&p, "-");
> > if (p != NULL)
> > goto bail;
> >
> > > + if (mutt_atoi(tstr, &time.tm_year, 0) < 0)
> >
> > And then here, I'd use y (and in the next two calls, m and d).
> > > + {
> > > + p = tstr;
> >
> > Setting p here seems dead code; am I missing something?
>
> No, this was a goofup on my part. I failed to notice that it was
> resetting p so that it could print a readable error message:
>
> bail:
> muttdbg(5, "invalid number: '%s'", p);
> return NULL;
>
> I will rework the function to that it passes p by reference and updates
> it on error.
[...]
> > > +bail:
> > > + return -1;
> >
> > Why not return -1 directly instead of goto?
>
> Yeah this was also because of the refactor, trying the keep the code the
> exact same. I'll change this too.
Actually, I see value in having the refactor essentially move code.
Maybe the improvement to return early can go in a separate commit.
(I guess you'll figure out the best way forward.)
Have a lovely night!
Alex
--
<https://www.alejandro-colomar.es>
signature.asc
Description: PGP signature
