Re: [PATCH] Add key expiration date to key menu for classic PGP and GPGME.

--qe7r7hs4ky47nhre
Content-Type: text/plain; protected-headers=v1; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
From: Alejandro Colomar <[email protected]>
To: [email protected]
Subject: Re: [PATCH] Add key expiration date to key menu for classic PGP and
 GPGME.
Message-ID: <akw0fEmrhukLILZr@devuan>
References: <[email protected]>
 <aku5OQsmkciVoO_M@devuan>
 <[email protected]>
MIME-Version: 1.0
In-Reply-To: <[email protected]>

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 =3D (const crypt_key_t * const *) a;
> > > +  const crypt_key_t * const *t =3D (const crypt_key_t * const *) b;
> > > +  unsigned long ts =3D 0, tt =3D 0;
> >=20
> > Why don't we use time_t?  Can it overflow?
>=20
> The field according to the GPGME doc is a unsigned long:
>=20
> https://www.gnupg.org/documentation/manuals/gpgme/Key-objects.html
>=20
>   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. No=
te
> that an invalid timestamp indicates a bug in the engine.
>=20
>   unsigned long int expires
>=20
>   This is the expiration timestamp of the subkey, or 0 if the subkey does
> not expire.
>=20
[...]
> > > +    strncpy(tstr, p, 11);
> > > +    tstr[4] =3D '\0';
> > > +    tstr[7] =3D '\0';
> >=20
> > strncpy(3) should never be used for copying strings with truncation.
> >=20
> > Some of its issues are:
> >=20
> > -  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.
>=20
> 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 comm=
it.

Yup; certainly.

> > In this case, I'd use the following to replace the code above:
> >=20
> > 	const char  *y, *m, *d;
> >=20
> > 	if (strlen(p) >=3D sizeof(tstr))
> > 		goto bail;
> > 	strcpy(tstr, p);
> > 	p =3D tstr;
> > 	y =3D strsep(&p, "-");
> > 	m =3D strsep(&p, "-");
> > 	d =3D strsep(&p, "-");
> > 	if (p !=3D NULL)
> > 		goto bail;
> >=20
> > > +    if (mutt_atoi(tstr, &time.tm_year, 0) < 0)
> >=20
> > And then here, I'd use y (and in the next two calls, m and d).
> > > +    {
> > > +      p =3D tstr;
> >=20
> > Setting p here seems dead code; am I missing something?
>=20
> 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:
>=20
> bail:
>   muttdbg(5, "invalid number: '%s'", p);
>   return NULL;
>=20
> I will rework the function to that it passes p by reference and updates
> it on error.

[...]

> > > +bail:
> > > +  return -1;
> >=20
> > Why not return -1 directly instead of goto?
>=20
> 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

--=20
<https://www.alejandro-colomar.es>

--qe7r7hs4ky47nhre
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEES7Jt9u9GbmlWADAi64mZXMKQwqkFAmpMNQUACgkQ64mZXMKQ
wqk58w/+PheAt3WBkoJqPFx91MM7cRq3WXYXKc6/qOuVcybaVyU/gyaWHelOMAjg
m+yKDH7Sp7N+iEEnUZvJ5nQJ1P+2wVUs8/XaALy+hm5r1s6jHn72bi8s+rWOxFED
EQUdXeROTDE/91fY+zgF/OeJrQliO+Yp/Id2DAMl2blAce3Ci2pbXdWHgHLb3AI5
qBx1ToESgE/kLMybGRm0epAYhoNG8fagUd/4dbYhn5ysi1l1ODkiAjgP0XKr7i87
cgWiwCD+fIgaaGhEIaVbdAHRcFlMYMnXl19uQYWjpN5vwkLpPWLWB+52mLnbnv/J
yLLlz8nAW60WsknMhJk0iYnR2XLcBZQNmuCQXlGDR1VkpTMbgg6JZMTTpIIMIc/K
HbPN7B7TLb8l/GPzplZDPWc5xvg8EK9ofu61wpr/j0q/TQJQFX1P+Ijb3ST3kFVX
p8Pq3u1UbR5yGkDQaetZDJopguKj6VWVK14ML5Uj1wYGhE2mgxLpdEG7LIagNZ1Q
G6nEIWkB/nuij12n5D5Zo71c2WsMCJHxRKXK96mNztaICupEYHpwy6Yr3mmX9ztK
4JVIncQ0geKiUiC1ZrImdtCxkQajSuZ/++P4YSsn6J9Q7h8kq9u+EZrXrz7NB1lr
+TkaZDWlbxpv4DClAbRrqdwQY5iLxajAxOhrBIq55WNM+mQMzrM=
=D46v
-----END PGP SIGNATURE-----

--qe7r7hs4ky47nhre--