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

--yqujhfyocjuc2r7e
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>
 <lotd7eh7thv3dvdggmuwdx3z7tfzwiulwmt2pglgq7zqyavi4n@x6p5nwmipiay>
MIME-Version: 1.0
In-Reply-To: <lotd7eh7thv3dvdggmuwdx3z7tfzwiulwmt2pglgq7zqyavi4n@x6p5nwmipiay>

On Wed, Mar 19, 2025 at 07:48:59PM +0100, Alejandro Colomar wrote:
> 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 coerc=
ion,
> > > > >   -b. expect to parse the initial portion of the string, silent c=
oercion,
> > > > >   -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 E=
RANGE
> > > >=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 treati=
ng
> > > a value > ULONG_MAX is equivalent to treating the value ULONG_MAX. Th=
ese
> > > are *normal* uses of strto[u]l[l]. Often it is the programmer's intent
> > > that the values "4294967297" and "4294967295" produce the same behavi=
our
> > > (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 ERAN=
GE
> > >    -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-desi=
gned,
> > > 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 bas=
es
> > > > 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 ER=
ANGE)
> > 			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 o=
n 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, &en=
dptr, 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 (*endpt=
r))
> > 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 l=
evel,
> > lib/nproc.c-411-             since we can't determine the nesting level=
 from 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 th=
at
> > could be true is if the base is unsupported, and 10 is necessarily
>=20
> s/true/equal/
>=20
> > 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, &=
endptr, 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 (*end=
ptr))
> > 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=
 level,
> > lib/omp-init.c-57-             since we can't determine the nesting lev=
el 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 (=
optarg, &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' && =
*nplurals <=3D '9')
> > gettext-tools/src/msgl-check.c:379:            nplurals_value =3D strto=
ul (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 _(=
"invalid nplurals value");
> > gettext-tools/src/msgl-check.c-383-              char *help =3D plural_=
help (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 x=
asprintf ("%s\n%s", msg, help);
> > gettext-tools/src/msgl-check.c-388-                  xeh->xerror (CAT_S=
EVERITY_ERROR, header, NULL, 0, 0, true,
> > gettext-tools/src/msgl-check.c-389-                               msgex=
t);
> > 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_SEV=
ERITY_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");

(Actually, strtou(3) skips leading space, so we don't really need this
 at all.)

> > 	nplurals_value =3D strtou_noneg(nplurals, (char **) &end, 10, 0, ULONG=
_MAX);
> > 	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 s=
trrchr (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 strto=
ul (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: <filenam=
e>:<number>" type comment.  */
> > gettext-tools/src/read-stringtable.c-564-          *last_colon =3D '\0';
> > gettext-tools/src/read-stringtable.c-565-          catalog_reader_seen_=
comment_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_co=
mment (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 mas=
k, so that
> > > > > the error conditions "The converted value was out of range and ha=
s been
> > > > > coerced" and "The given string contains characters that did not g=
et converted"
> > > > > can be both returned together, without conflicting.
> > > >=20
> > > > Because it is theoretical conditions that a real program never want=
s,
> > > > let's not do that.
> > >=20
> > > If you don't want to do that, I can only repeat what I said in the pr=
evious
> > > mail: The proposal *does not achieve the goal* of avoiding the most c=
ommon
> > > programmer mistakes. For a robust API, the success test should *only*=
 involve
> > > 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
>=20
>=20
> --=20
> <https://www.alejandro-colomar.es/>



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

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

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

iQIzBAABCgAdFiEES7Jt9u9GbmlWADAi64mZXMKQwqkFAmfbE0YACgkQ64mZXMKQ
wql8OA//R0A+x9q24jLifsD6ntu5D+DwxQd6lyi9hSt7+KANl5rGf7fmgGAU+bJP
PYJ0FRFEwkcuca4BTgVaxp2SkR+jDR7Nh7D1sk6VL8mBVf1X4lzd5uA0ZMOIOLcY
ZbrAMWW/hy6Th/rBlNi8jNbzaveSHE2YUYydCehxG8/U6KiyA7qqaK0mlDqbLx4Q
72rEK5RSHYrapYNK02uNBOyGTrZ6EokYv6g1NGNbjX6IgdnhXie3A1UVq5TM0JVH
3ryHYoNIcvLx2agQ+Yb6QphasP8w9Mzdvtfd9YlHaXv9V/736UsRGN7cPmNfevko
mXorXFqSMTYbwhIBfc60yiYUvslufTWHEe8+SX6jjygVPeRm/Lk7+13tajaq3xAq
bKK/24nopbiKOtwA+3zwZS8a178G2eaR9Ke2szeitESCjvaxLulj2Zvr/10vAJlj
XXdtHfEjlJalczGZximn7SzxkB9h1KzRw9tHkIYHyiljh6xqdvcdo3yqvyK41WXe
UVeSu3P/nNZKDDCx9KCLIXqVKDCA1c14e2BAQcCdaY7p89x/UNo3E3ktWS2oSN+q
X4QhNuLgbFu86RdRhWmwxPGbheWYGRRvi+XSNXlCZxFBEaNgCXVOLMZ8czUSacD4
10qT7ZZJl07TXWIz37DPAmPjcliGx3Tl5KWQK4Tz3qp0m1jQS9g=
=KpmZ
-----END PGP SIGNATURE-----

--yqujhfyocjuc2r7e--