Re: alx-0008 - Standardize strtoi(3) and strtou(3) from NetBSD

--6mfxviq2dael3gtj
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], [email protected], 
	[email protected], [email protected], christos <[email protected]>, 
	=?utf-8?B?xJBvw6BuIFRy4bqnbiBDw7RuZw==?= Danh <[email protected]>, Paul Eggert <[email protected]>, 
	Eli Schwartz <[email protected]>, Guillem Jover <[email protected]>, 
	Iker Pedrosa <[email protected]>, Michael Vetter <[email protected]>, Robert Elz <[email protected]>, 
	[email protected], Sam James <[email protected]>, "Serge E. Hallyn" <[email protected]>
Subject: Re: alx-0008 - Standardize strtoi(3) and strtou(3) from NetBSD
References: <mgcfwxfmv3kpfnkkf6uj63kx5tdzl64p2zg2us4ntsu6q5xkwj@k52z5yyiywoo>
 <18739733.sWSEgdgrri@nimes>
 <mvwnrmk2xf45ivyk4kzxdxuwdk67666yt3kwafck6vo4vq2lru@wkqmoqsacqkf>
 <3237498.fEcJ0Lxnt5@nimes>
 <jx4664ishtl34eg2npdrv5fkfdiczqnlq3vjuacjrupjvh377x@gddcftzgwmfq>
MIME-Version: 1.0
In-Reply-To: <jx4664ishtl34eg2npdrv5fkfdiczqnlq3vjuacjrupjvh377x@gddcftzgwmfq>

On Wed, Mar 19, 2025 at 04:26:11PM +0100, Alejandro Colomar wrote:
> Hi Bruno,
>=20
> On Wed, Mar 19, 2025 at 01:15:30AM +0100, Bruno Haible wrote:
> > Alejandro Colomar wrote:
> > > > It would be useful to show how a success test looks like, after
> > > >     strtoi (s, &end, base, min, max, &status)
> > > > for each of the four frequent use-cases:
> > > >   -a. expect to parse the initial portion of the string, no coercio=
n,
> > > >   -b. expect to parse the initial portion of the string, silent coe=
rcion,
> > > >   -c. expect to parse the entire string, no coercion,
> > > >   -d. expect to parse the entire string, silent coercion.
> > > >=20
> > > > AFAICS, the success tests are:
> > > >   -a. status =3D=3D 0 || status =3D=3D ENOTSUP
> > >=20
> > > Correct.
> > >=20
> > > >   -b. status =3D=3D 0 || status =3D=3D ENOTSUP || status =3D=3D ERA=
NGE
> > >=20
> > > Correct (but most likely a bug).
>=20
> Actually, now I remember that status can be NULL, in which case it's not
> reported.  This is a case where you could check for errors with a
> simpler expression:
>=20
> 	end !=3D str
>=20
> but (status =3D=3D 0 || status =3D=3D ENOTSUP || status =3D=3D ERANGE) is=
 still a
> reasnoable one.
>=20
> like you can do with strtol(3), but with portability guarantess
> regarding EINVAL, because strtoi(3bsd) always writes *endp (if nonnull).
>=20
> I need to update the specification to mention that status can be NULL.
>=20
> > >=20
> > > >   -c. status =3D=3D 0
> > >=20
> > > Correct.
> > >=20
> > > >   -d. status =3D=3D 0 || (status =3D=3D ERANGE && end > s && *end =
=3D=3D '\0')
> > >=20
> > > You don't need end>s, because that would preclude ERANGE.
> > >=20
> > > 	status =3D=3D 0 || (status =3D=3D ERANGE && end =3D=3D '\0')
> > >=20
> > > Aaand, most likely a bug.
> >=20
> > Cases b. and d. are not bugs. Often, the programmer knows that treating
> > a value > ULONG_MAX is equivalent to treating the value ULONG_MAX. These
> > are *normal* uses of strto[u]l[l]. Often it is the programmer's intent
> > that the values "4294967297" and "4294967295" produce the same behaviour
> > (the same error message, for example).
>=20
> If you want ULONG_MAX + 1 to be treated like ULONG_MAX, and both
> result in an error, then you should probably clamp at ULONG_MAX - 1,
> and consider anything above an error.
>=20
> > It is for these cases that your specification contains the clamping /
> > coercion behaviour.
> >=20
> > Now, when you look at the table of success tests:
> >=20
> >    -a. status =3D=3D 0 || status =3D=3D ENOTSUP
> >    -b. status =3D=3D 0 || status =3D=3D ENOTSUP || status =3D=3D ERANGE
> >    -c. status =3D=3D 0
> >    -d. status =3D=3D 0 || (status =3D=3D ERANGE && *end =3D=3D '\0')
> >=20
> > it is immediately clear that the status return convention is ill-design=
ed,
> > because the returned 'status' is not the only thing a programmer has to=
 test
> > after calling the function.
> >=20
> > > Cases b and d are not real, IMO.  I have never seen code where that is
> > > wanted, AFAIR, and I analyzed the entire Debian and NetBSD code bases
> > > looking precisely for that usage.
> >=20
> > I disagree.
>=20
> I didn't find any occurence of 'd' in calls to strtoi(3)/strtou(3).
> I didn't analyze calls to strtol(3) et al.
>=20
> > Any use of strtoul that does not test errno wants overflow
> > to be mapped to ULONG_MAX, that is, is in case b. or d.
> > Just looking in gnulib and gettext, I find already 6 occurrences:
> >   gnulib/lib/getaddrinfo.c:299
>=20
> lib/getaddrinfo.c-297-          if (!(*servname >=3D '0' && *servname <=
=3D '9'))
> lib/getaddrinfo.c-298-            return EAI_NONAME;
> lib/getaddrinfo.c:299:          port =3D strtoul (servname, &c, 10);
> lib/getaddrinfo.c-300-          if (*c || port > 0xffff)
> lib/getaddrinfo.c-301-            return EAI_NONAME;
> lib/getaddrinfo.c-302-          port =3D htons (port);
>=20
> You could remove the preceding conditional if you don't want to avoid
> leading whitespace.  You could merge that into the strtou(3) call, which
> would report ECANCELED for non-numeric input).  Except that a negative
> number is silently converted to a positive large value.  This is why I
> use a wrapper function strtou_noneg() that rejects negative numbers.
>=20
> You could rewrite it as:
>=20
> 	port =3D strtou_noneg(servname, NULL, 10, 0, UINT16_MAX, &status);
> 	if (status !=3D 0)
> 		return EAI_NONAME;
> 	port =3D htons(port);
>=20
> where strtou_noneg() is:
>=20
> 	uintmax_t
> 	strtou_noneg(const char *s, char **restrict endp, int base,
> 	    uintmax_t min, uintmax_t max, int *restrict status)
> 	{
> 		int  st;
>=20
> 		if (status =3D=3D NULL)
> 			status =3D &st;
> 		if (strtoi(s, endp, base, 0, 1, status) =3D=3D 0 && *status =3D=3D ERAN=
GE)
> 			return min;
>=20
> 		return strtou(s, endp, base, min, max, status);
> 	}
>=20
> I think this is not one case where you want silent saturation.  You're
> indeed doing range checks [0, UINT16_MAX].
>=20
> >   gnulib/lib/nproc.c:402
>=20
> lib/nproc.c-383-/* Parse OMP environment variables without dependence on =
OMP.
> lib/nproc.c-384-   Return 0 for invalid values.  */
> lib/nproc.c-385-static unsigned long int
> lib/nproc.c:386:parse_omp_threads (char const* threads)
> lib/nproc.c-387-{
>=20
> ...
>=20
> lib/nproc.c-398-  /* Convert it from positive decimal to 'unsigned long'.=
  */
> lib/nproc.c-399-  if (c_isdigit (*threads))
> lib/nproc.c-400-    {
> lib/nproc.c-401-      char *endptr =3D NULL;
> lib/nproc.c:402:      unsigned long int value =3D strtoul (threads, &endp=
tr, 10);
> lib/nproc.c-403-
> lib/nproc.c-404-      if (endptr !=3D NULL)
> lib/nproc.c-405-        {
> lib/nproc.c-406-          while (*endptr !=3D '\0' && c_isspace (*endptr))
> lib/nproc.c-407-            endptr++;
> lib/nproc.c-408-          if (*endptr =3D=3D '\0')
> lib/nproc.c-409-            return value;
> lib/nproc.c-410-          /* Also accept the first value in a nesting lev=
el,
> lib/nproc.c-411-             since we can't determine the nesting level f=
rom env vars.  */
> lib/nproc.c-412-          else if (*endptr =3D=3D ',')
> lib/nproc.c-413-            return value;
> lib/nproc.c-414-        }
> lib/nproc.c-415-    }
>=20
> First of all, the endptr!=3DNULL test seems misplaced.  The only way that
> could be true is if the base is unsupported, and 10 is necessarily

s/true/equal/

> supported.  You should remove the initialization '=3D NULL', and the
> check, since both are dead code, IIRC.  That's one of the things you
> don't need to care with strtoi(3), because it _always_ sets *endp.
>=20
> And you could probably remove the isdigit test by calling
> strtou_noneg().
>=20
> This could be something like this (fixing the bugs reported above):
>=20
> 	char    *end;
> 	u_long  value;
>=20
> 	value =3D strtou_noneg(threads, &end, 10, 0, ULONG_MAX, NULL);
> 	if (end !=3D threads) {
> 		end +=3D strspn(end, " \t\n");
> 		if (streq(end, "")
> 			return value;
> 		if (strprefix(end, ","))
> 			return value;
> 	}
>=20
>=20
> This is one case where you seem to silently ignore saturation.  Why
> don't you have any diagnostic message?
>=20
> >   gnulib/lib/omp-init.c:48
>=20
> lib/omp-init.c-47-      char *endptr =3D NULL;
> lib/omp-init.c:48:      unsigned long int value =3D strtoul (threads, &en=
dptr, 10);
> lib/omp-init.c-49-
> lib/omp-init.c-50-      if (endptr !=3D NULL)
> lib/omp-init.c-51-        {
> lib/omp-init.c-52-          while (*endptr !=3D '\0' && c_isspace (*endpt=
r))
> lib/omp-init.c-53-            endptr++;
> lib/omp-init.c-54-          if (*endptr =3D=3D '\0')
> lib/omp-init.c-55-            return value;
> lib/omp-init.c-56-          /* Also accept the first value in a nesting l=
evel,
> lib/omp-init.c-57-             since we can't determine the nesting level=
 from env vars.  */
> lib/omp-init.c-58-          else if (*endptr =3D=3D ',')
> lib/omp-init.c-59-            return value;
> lib/omp-init.c-60-        }
>=20
> This seems identical to the previous case.
>=20
> >   gettext/gettext-tools/src/msgfmt.c:287
>=20
> gettext-tools/src/msgfmt.c-286-          char *endp;
> gettext-tools/src/msgfmt.c:287:          size_t new_align =3D strtoul (op=
targ, &endp, 0);
> gettext-tools/src/msgfmt.c-288-
> gettext-tools/src/msgfmt.c-289-          if (endp !=3D optarg)
> gettext-tools/src/msgfmt.c-290-            alignment =3D new_align;
>=20
> This code will misbehave badly on platforms where size_t is narrower
> than u_long.  Consider the case where you parse a high u_long, let's say
> SIZE_MAX + 1ul.  It will be converted to 1.  There's a bug due to a
> missing range check (but orthogonal to saturation).
>=20
> You also don't reject negative numbers, which I expect to be a bug,
> connected with the one from above.
>=20
> This could be rewritten to (fixing the bugs reported above):
>=20
> 	char    *end;
> 	size_t  new_align;
>=20
> 	new_align =3D strtou_noneg(optarg, &end, 0, 0, SIZE_MAX, NULL);
> 	if (optarg !=3D end)
> 		alignment =3D new_align;
>=20
> This seems another case where you silently saturate.  Why don't you have
> a diagnostic message for invalid input?
>=20
> >   gettext/gettext-tools/src/msgl-check.c:379
>=20
> gettext-tools/src/msgl-check.c-374-          while (*nplurals !=3D '\0' &=
& c_isspace ((unsigned char) *nplurals))
> gettext-tools/src/msgl-check.c-375-            ++nplurals;
> gettext-tools/src/msgl-check.c-376-          endp =3D nplurals;
> gettext-tools/src/msgl-check.c-377-          nplurals_value =3D 0;
> gettext-tools/src/msgl-check.c-378-          if (*nplurals >=3D '0' && *n=
plurals <=3D '9')
> gettext-tools/src/msgl-check.c:379:            nplurals_value =3D strtoul=
 (nplurals, (char **) &endp, 10);
> gettext-tools/src/msgl-check.c-380-          if (nplurals =3D=3D endp)
> gettext-tools/src/msgl-check.c-381-            {
> gettext-tools/src/msgl-check.c-382-              const char *msg =3D _("i=
nvalid nplurals value");
> gettext-tools/src/msgl-check.c-383-              char *help =3D plural_he=
lp (nullentry);
> gettext-tools/src/msgl-check.c-384-
> gettext-tools/src/msgl-check.c-385-              if (help !=3D NULL)
> gettext-tools/src/msgl-check.c-386-                {
> gettext-tools/src/msgl-check.c-387-                  char *msgext =3D xas=
printf ("%s\n%s", msg, help);
> gettext-tools/src/msgl-check.c-388-                  xeh->xerror (CAT_SEV=
ERITY_ERROR, header, NULL, 0, 0, true,
> gettext-tools/src/msgl-check.c-389-                               msgext);
> gettext-tools/src/msgl-check.c-390-                  free (msgext);
> gettext-tools/src/msgl-check.c-391-                  free (help);
> gettext-tools/src/msgl-check.c-392-                }
> gettext-tools/src/msgl-check.c-393-              else
> gettext-tools/src/msgl-check.c-394-                xeh->xerror (CAT_SEVER=
ITY_ERROR, header, NULL, 0, 0, false,
> gettext-tools/src/msgl-check.c-395-                             msg);
> gettext-tools/src/msgl-check.c-396-
> gettext-tools/src/msgl-check.c-397-              seen_errors++;
> gettext-tools/src/msgl-check.c-398-            }
>=20
> You could get rid of a lot of code preceding the strtoul(3) call by
> calling strtou_noneg() instead.  And strspn(3) would also help.
>=20
> 	nplurals +=3D strspn(nplurals, " \t\n");
> 	nplurals_value =3D strtou_noneg(nplurals, (char **) &end, 10, 0, ULONG_M=
AX);
> 	if (nplurals =3D=3D end)
> 		...
>=20
> On the other hand, I also wonder why you don't diagnose invalid input.
> Why is -10 an "invalid nplurals value", but ULONG_MAX+10 is a valid
> (albeit clamped) one?
>=20
> All of these cases look like missing error handling, IMO.
>=20
> >   gettext/gettext-tools/src/read-stringtable.c:561
>=20
> gettext-tools/src/read-stringtable.c-553-    {
> gettext-tools/src/read-stringtable.c-554-      char *last_colon;
> gettext-tools/src/read-stringtable.c-555-      unsigned long number;
> gettext-tools/src/read-stringtable.c-556-      char *endp;
> gettext-tools/src/read-stringtable.c-557-
> gettext-tools/src/read-stringtable.c-558-      if (strlen (line) >=3D 6 &=
& memcmp (line, "File: ", 6) =3D=3D 0
> gettext-tools/src/read-stringtable.c-559-          && (last_colon =3D str=
rchr (line + 6, ':')) !=3D NULL
> gettext-tools/src/read-stringtable.c-560-          && *(last_colon + 1) !=
=3D '\0'
> gettext-tools/src/read-stringtable.c:561:          && (number =3D strtoul=
 (last_colon + 1, &endp, 10), *endp =3D=3D '\0'))
> gettext-tools/src/read-stringtable.c-562-        {
> gettext-tools/src/read-stringtable.c-563-          /* A "File: <filename>=
:<number>" type comment.  */
> gettext-tools/src/read-stringtable.c-564-          *last_colon =3D '\0';
> gettext-tools/src/read-stringtable.c-565-          catalog_reader_seen_co=
mment_filepos (catr, line + 6, number);
> gettext-tools/src/read-stringtable.c-566-        }
> gettext-tools/src/read-stringtable.c-567-      else
> gettext-tools/src/read-stringtable.c-568-        catalog_reader_seen_comm=
ent (catr, line);
> gettext-tools/src/read-stringtable.c-569-    }
>=20
> Let me try to rewrite it for readability first.
>=20
> 	char        *filepos, *last_colon;
> 	u_long      n;
> 	const char  *numstr, *end;
>=20
> 	filepos =3D strprefix(line, "File: ") ?: (char []){""};
> 	last_colon =3D strrchr(filepos, ':') ?: (char []){":"};
> 	numstr =3D strprefix(last_colon, ":");
>=20
> 	n =3D strtoul(numstr, (char **) &end, 10);
> 	if (numstr !=3D end && streq(end, "")) {
> 		/* A "File: <filename>:<number>" type comment.  */
> 		strcpy(last_colon, "");
> 		catalog_reader_seen_comment_filepos(catr, filepos, n);
>=20
> 	} else {
> 		catalog_reader_seen_comment(catr, line);
> 	}
>=20
> You're forgetting about negative numbers?  Or are you certain that they
> can't happen?  How about huge values?  Assuming you want compatible code
> calling strtou():
>=20
> 	n =3D strtou(numstr, (char **) &end, 10, 0, ULONG_MAX, NULL);
> 	if (status =3D=3D 0 || status =3D=3D ENOTSUP || status =3D=3D ERANGE) {
> 		...
> 	} else {
> 		...
> 	}
>=20
> But again, I wonder why you don't do range checks.
>=20
> > > > I would therefore propose to change the status value to a bit mask,=
 so that
> > > > the error conditions "The converted value was out of range and has =
been
> > > > coerced" and "The given string contains characters that did not get=
 converted"
> > > > can be both returned together, without conflicting.
> > >=20
> > > Because it is theoretical conditions that a real program never wants,
> > > let's not do that.
> >=20
> > If you don't want to do that, I can only repeat what I said in the prev=
ious
> > mail: The proposal *does not achieve the goal* of avoiding the most com=
mon
> > programmer mistakes. For a robust API, the success test should *only* i=
nvolve
> > testing the returned 'status', nothing else.
>=20
> Let's discuss this after your responses to the above.
>=20
> >=20
> > Bruno
> >=20
> >=20
> >=20
> >=20
>=20
> --=20
> <https://www.alejandro-colomar.es/>



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

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

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

iQIzBAABCgAdFiEES7Jt9u9GbmlWADAi64mZXMKQwqkFAmfbEYgACgkQ64mZXMKQ
wqmq8g/+LWZ+gHSyWy3mS1wSFUtpRotvppJe09hTrQmNK5wBl2XBDooK7/XyS+qI
J4SnxV6kWHLGIouefvz5G5TyOmHWYANJPNsAM93z+cCCgPgqPNx7ywiimJY+vt8Y
MESwQrc2Y64QH0PYpcfJptZQxzEXoq3OHXTW6P+L058Zj5yl0K4xhipi4S+KJG4q
5azVfWREFJdtKi73Q0Ctu2tgjs6BzHoAOrajieyRIBRCF167Mera3pUsIMcHWhcw
+P0rOhuUHsBoTkHlU48Xg+vZ2Im31Sv9FgrtqhntUWU0/7M+epCsIbaog1nnbPru
/eHAQO9sEO9zyPCOMHPSrw25i8tNNEfMEMxU06uF+R7B6Lpgum7LgSJYCyRVMv9b
jilL/Sshmk8RVECMynruYjmbwPXR69cbWxkdaX3zMk1WCc7wqdBIp3kVbhBIpWRi
wqZRVjnqD9xcwvjvn6ZyRlE4ubweQLLsluK+JrUhwPcgqG7/BBTHM6zoggP59fc5
2gH/gTpOngzWQl1nMvMMCNy5GlwCzUhPI1ci6f2NtCaIQlSAWBtBx2tlAsY+/662
hQm6oxe0UOdHVKnIsX052V/tTJsBjwi0rKBj6Os7k159Ecvxumx3ubz9Xb2iaZyl
cT4Ymmvu9DzQcKt+swtvI53zX1xoWgiJotouTo+DRcl8CuubgKI=
=wlmx
-----END PGP SIGNATURE-----

--6mfxviq2dael3gtj--