Re: [PATCH v1] xstrtol: Remove dead code

--fzbrwa7e37dq4tly
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>
MIME-Version: 1.0
In-Reply-To: <kul6qwyrvq2xdevjjkkblsalg7ycwg6x7ey725kans5myqjcai@3jm56c5famlt>

[CC -=3D Andrew, per explicit request]

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,

Hi Bruno,

> > > 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, POSIX=
).
> >=20
> > However, there's a difference between what the standards say and what t=
he
> > systems actually do. The Gnulib documentation contains thousands of exa=
mples
> > of such differences.
> >=20
> > Gnulib therefore (almost) never assumes that there are no possible errno
> > 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 implementa=
tions
> >     of strtod() do use malloc().
> >=20
> > So, what you call "dead code", I call "defensive programming". I would =
not
> > like to apply this patch.

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,

We'd need to know the precise specification of that system that can set
errno =3D ENOMEM.

Is *endp guaranteed to be set?  Or may it be unset (as happens with
EINVAL)?

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:

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;

	e =3D NULL;
	ep =3D &e;

	if (status =3D=3D NULL)
		status =3D &st;

	if (base !=3D 0 && (base < 2 || base > 36)) {
		*status =3D EINVAL;
		return MAX(min, MIN(max, 0));
	}

	errno_saved =3D errno;
	errno =3D 0;

	n =3D strtoimax(s, ep, base);

	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;

	errno =3D errno_saved;

	if (endp !=3D NULL)
		*endp =3D e;

	return MAX(min, MIN(max, n));
}

Does this make sense?

Have a lovely night!
Alex

>=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_005fr=
=2Ehtml
> >=20
> >=20
> >=20
>=20
> --=20
> <https://www.alejandro-colomar.es/>



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

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

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

iQIzBAABCgAdFiEE6jqH8KTroDDkXfJAnowa+77/2zIFAmaZk7AACgkQnowa+77/
2zKWHxAAljv8KWAeVntrtFM2wX4GryxIz39kpN1DurTKzAPKjRMrCFQXiDf2ZMf/
ExVpkAHFMKjLlD2ENhfaLcDXr8W5nLuHYwzeySgG+rH7Q6QC8uqR0UNyq+nKID0j
RvQiEH6KItp9MaxkHtUUHIu5B436FBE3czqgOTdfVwt9GqwjL9lbjCOEBBtHshYK
8QGcDZmnktcPVHEB4bJnsLvEHphIrevt/o7d1aj5G3maN1kIOiSiK8TKfo3MalA6
+lz8T3TeRHDm/gDTRiXmGHZtB4KBHw+/5zcGBO8W2jvilVm6rgvwQgHdxUkpZgve
0x32k0rbKBOY+nQtAnO1dwgl7/rrv9BU2gY/+tlN86RyMMUCcFwVss02JayreyhK
qwl2gULtR60sk8ckASIeyg1K9lZvRmQHrYhAx2htdI0WjSeJU2SAUNxVdWK8hEyX
M068SZ0WwZO0rjIoS4SDE8JSWlBzc92R+exTqzx0A7IQ2ZA4WQQ0baB5VNH4caIL
IgvsBSvA2tpZeU79PnmUUHJP+W++blhOmg1bCasRPoSu4ih6652QHSJROp4XaXsz
ZJdm9x6VNVv9FQFJBwjW/mAR4Ygn7T+a/8hyrnkcgGLxUoRuZ5ojifpwgKPdfn3d
FX+PfbAawZY4tE4VzZ+jahs+RQOClTQiJi1s3a2RTv/FlSSGGag=
=amSz
-----END PGP SIGNATURE-----

--fzbrwa7e37dq4tly--