Re: [PATCH v1] xstrtol: Remove dead code

--a4hyyi26hqfgkx32
Content-Type: text/plain; protected-headers=v1; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
From: Alejandro Colomar <[email protected]>
To: Bruno Haible <[email protected]>
Cc: [email protected], Paul Eggert <[email protected]>, 
	=?utf-8?B?xJBvw6BuIFRy4bqnbiBDw7RuZw==?= Danh <[email protected]>, Eli Schwartz <[email protected]>, Sam James <[email protected]>, 
	Serge Hallyn <[email protected]>, Iker Pedrosa <[email protected]>, 
	Michael Vetter <[email protected]>, [email protected]
Subject: Re: [PATCH v1] xstrtol: Remove dead code
References: <[email protected]>
 <8009158.qOBuL9xsDt@nimes>
 <kul6qwyrvq2xdevjjkkblsalg7ycwg6x7ey725kans5myqjcai@3jm56c5famlt>
 <2licxuxsw37hpyss5izkqu6x4lfwcduxwbrgw7a4fqaibydwlx@o6y73xqtyrnx>
MIME-Version: 1.0
In-Reply-To: <2licxuxsw37hpyss5izkqu6x4lfwcduxwbrgw7a4fqaibydwlx@o6y73xqtyrnx>

On Fri, Jul 19, 2024 at 12:14:19AM GMT, Alejandro Colomar wrote:
> [CC -=3D Andrew, per explicit request]
>=20
> On Thu, Jul 18, 2024 at 11:25:11PM GMT, Alejandro Colomar wrote:
> > On Thu, Jul 18, 2024 at 11:09:40PM GMT, Bruno Haible wrote:
> > > Hi Alejandro,
>=20
> Hi Bruno,
>=20
> > > > strtol(3) has a limited set of possible states:
> > > > ...
> > > > The condition '*endp !=3D s && errno !=3D 0 && errno !=3D ERANGE' is
> > > > unreachable.  The only errno possible if '*endp !=3D s' is ERANGE.
> > >=20
> > > Such a statement can be true if you look at the standards (ISO C, POS=
IX).
> > >=20
> > > However, there's a difference between what the standards say and what=
 the
> > > systems actually do. The Gnulib documentation contains thousands of e=
xamples
> > > of such differences.
> > >=20
> > > Gnulib therefore (almost) never assumes that there are no possible er=
rno
> > > values besides the ones listed in the standards.
> > >   - Some systems return "wrong" errno values. Example: [1]
> > >   - Some systems fail with ENOMEM when memory is tight. Who says that
> > >     an implementation of strtol() cannot use malloc() ? Some implemen=
tations
> > >     of strtod() do use malloc().
> > >=20
> > > So, what you call "dead code", I call "defensive programming". I woul=
d not
> > > like to apply this patch.
>=20
> On the other hand, I'm not sure that defensive programming is valid in
> this case.  I already discussed this topic with Serge (but we didn't
> have in mind any specific implementation) some time ago, and,
>=20
> We'd need to know the precise specification of that system that can set
> errno =3D ENOMEM.
>=20
> Is *endp guaranteed to be set?  Or may it be unset (as happens with
> EINVAL)?
>=20
> If it is allowed to keep *endp unset, then the first `if (*p =3D=3D s)`
> would already be UB.  And for truly being defensive, we'd need to assume
> that it may leave *endp unset.  Thus we cannot read that until we know
> that no errno has been set.  But then comes the problem that some
> systems set EINVAL on what strtoi(3) calls ECANCELED.  To work with all
> of those systems, we'd probably need to pass a dummy NULL to make sure
> we can inspect *endp regardless of strtol(3) having set it or not:
>=20
> intmax_t
> strtoi(char *s, char **restrict endp, int base,
>     intmax_t min, intmax_t max, int *restrict status)
> {
> 	int        errno_saved, st;
> 	char       *e;
> 	char       **ep;
> 	intmax_t   n;
>=20
> 	e =3D NULL;
> 	ep =3D &e;
>=20
> 	if (status =3D=3D NULL)
> 		status =3D &st;
>=20
> 	if (base !=3D 0 && (base < 2 || base > 36)) {
> 		*status =3D EINVAL;
> 		return MAX(min, MIN(max, 0));
> 	}
>=20
> 	errno_saved =3D errno;
> 	errno =3D 0;
>=20
> 	n =3D strtoimax(s, ep, base);
>=20
> 	if (errno =3D=3D ERANGE || n < min || n > max)
> 		*status =3D ERANGE;
> 	else if (e =3D=3D s && (errno =3D=3D 0 || errno =3D=3D EINVAL))
> 		*status =3D ECANCELED;
> 	else if (errno !=3D 0)
> 		*status =3D errno;
> 	else if (*e !=3D '\0')
> 		*status =3D ENOTSUP;
> 	else
> 		*status =3D 0;

Hmm, bug there.  I should have written:

	if (errno =3D=3D ERANGE)
		*status =3D ERANGE;
	else if (e =3D=3D s && (errno =3D=3D 0 || errno =3D=3D EINVAL))
		*status =3D ECANCELED;
	else if (errno !=3D 0)
		*status =3D errno;
	else if (n < min || n > max)
		*status =3D ERANGE;
	else if (*e !=3D '\0')
		*status =3D ENOTSUP;
	else
		*status =3D 0;

>=20
> 	errno =3D errno_saved;
>=20
> 	if (endp !=3D NULL)
> 		*endp =3D e;
>=20
> 	return MAX(min, MIN(max, n));
> }
>=20
> Does this make sense?
>=20
> Have a lovely night!
> Alex
>=20
> >=20
> > Makes sense.  I think we should document that possibility in the manual
> > page.  Maybe say that other errno values are possible in some systems?
> > Otherwise, it's already a hell of a function to take care of, and most
> > uses don't handle that possibility at the moment.  (Yet more reasons to
> > use a wrapper that returns -1 & sets errno on error, as the rest of
> > libc.)
> >=20
> > Would you send a patch?  (I'd write it myself, but you probably can
> > provide more info in the commit message.)
> >=20
> > Have a lovely night!
> > Alex
> >=20
> > >=20
> > > Bruno
> > >=20
> > > [1] https://www.gnu.org/software/gnulib/manual/html_node/getlogin_005=
fr.html
> > >=20
> > >=20
> > >=20
> >=20
> > --=20
> > <https://www.alejandro-colomar.es/>
>=20
>=20
>=20
> --=20
> <https://www.alejandro-colomar.es/>



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

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

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

iQIzBAABCgAdFiEE6jqH8KTroDDkXfJAnowa+77/2zIFAmaZpiIACgkQnowa+77/
2zKwIBAAoB8ekfPiexoy0lrjOAIfN91fU2kCR4iMVify9IQjmqLxjp7aMRHOuVeO
oQuKn8qXeVkz6/DR0kQny1NZcOjHU8xVi04RnBT+FbCnwBNRuWx7EM6kwIP61Wuy
pd6ZFeltPwA/8SvQY2uPGkjMvsidL1JN2XO8wvwYV8e7ptMr7FL7fbUgBy/rGBV6
CQe+4BCacn6tkJfb4PAFj/wO55XiYGjLFJgUct+LEBd2uFMr3r8oiGGCDrzVaRDs
CuI+cXhCKlW+l4xT6Z2HFhOTVpn9Go8y1tIaWdDFcNVeD3reQtCjZ1Kjj77OVpTx
fj9XQgEj1Fla8D3YFtTyWJl98mDcjGFgIdWLAqa5uJH4mHPlTumQ8im0hmVAB7MS
aQ62QdFNRFoQfxt7zar+ObZIhCko9rjluZwQKbI0UX/Xk0W/XUsSgzqzjjbq1+A5
kTYLvLKDmSrF4K+RPsCXCOAU+NsbRLvKitKMOWUjy1B/046EmGoYRTQCmdtnSU9m
WuJHrSJyvCX+9L7uok1JOYYVzAFqsdUtSlpJlInA+cJ6sZ11wpfZu2yWD4m5NJ0r
/mu6mTkbfkKAkH1s/cQ+G7mS5QFscang43m5MYca87pNv4Kqc7jXrJTcFBqFJ+TL
FqpcVVwBpuPQ6wl059ttd8j228vgUsCzRe3S8trCywmD2LL9c9c=
=rcOi
-----END PGP SIGNATURE-----

--a4hyyi26hqfgkx32--