Hi Bruno,
On Wed, Mar 19, 2025 at 10:59:26PM +0100, Bruno Haible wrote:
> Hi Alejandro,
>
> > > > > 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 coercion,
> > > > > -b. expect to parse the initial portion of the string, silent coercion,
> > > > > -c. expect to parse the entire string, no coercion,
> > > > > -d. expect to parse the entire string, silent coercion.
> > > > >
> > > > > AFAICS, the success tests are:
> > > > > -a. status == 0 || status == ENOTSUP
> > > >
> > > > Correct.
> > > >
> > > > > -b. status == 0 || status == ENOTSUP || status == ERANGE
> > > >
> > > > Correct (but most likely a bug).
> >
> > 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:
> >
> > end != str
> >
> > but (status == 0 || status == ENOTSUP || status == ERANGE) is still a
> > reasnoable one.
>
> Unfortunately, with a comment like this, you make things more complicated,
> not simpler. I was hoping for an API where success can be determined by
> looking at 'status' in all four cases, and the "simple" solution that you
> are recommending now is:
> -a. look at status
> -b. look at end
> -c. look at status
> -d. look at status AND end.
>
> > I need to update the specification to mention that status can be NULL.
>
> Why do so? This adds text to the specification, making the specification
> more complex (=> longer to understand, harder to remember). The ability
> to pass NULL for rstatus is not a useful feature.
Hmmm, in Debian, there's exactly one place where this happens, and it's
a test case, so I guess it's fine if we break it.
<https://sources.debian.org/src/mk-configure/0.37.0-2/tests/mkc_features/tool/test_features1.cxx/?hl=118#L118>
And there are 0 such calls in NetBSD trunk:
alx@devuan:~/src/bsd/netbsd/trunk$ find -type f \
| grep '\.[ch]$' \
| xargs grep -l '\<strto[iu]\>' \
| xargs pcre2grep -Mn '(?s)\bstrto[iu] *\([^;]*(NULL|0)\)';
So we could tighten the specification to require a non-null pointer.
I would be okay with that.
> > > > > -d. status == 0 || (status == ERANGE && end > s && *end == '\0')
> > > >
> > > > You don't need end>s, because that would preclude ERANGE.
> > > >
> > > > status == 0 || (status == ERANGE && end == '\0')
> > > >
> > > > Aaand, most likely a bug.
> > >
> > > 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).
> >
> > 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.
>
> I said it in the paragraph above: Often the programmer wants a smooth
> transition between "unreasonably large but not overflowing" values and
> "overflow". So that only one diagnostic is needed for both cases.
>
> 1) getaddrinfo.c
>
> > > 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
> >
> > lib/getaddrinfo.c-297- if (!(*servname >= '0' && *servname <= '9'))
> > lib/getaddrinfo.c-298- return EAI_NONAME;
> > lib/getaddrinfo.c:299: port = strtoul (servname, &c, 10);
> > lib/getaddrinfo.c-300- if (*c || port > 0xffff)
> > lib/getaddrinfo.c-301- return EAI_NONAME;
> > lib/getaddrinfo.c-302- port = htons (port);
> >
> > You could remove the preceding conditional if you don't want to avoid
> > leading whitespace.
>
> We don't want to allow leading whitespace here. We need to follow the
> getaddrinfo() spec [1].
Ok.
> At the same time, disallowing a leading '-' sign
> is a benefit as well. I consider it a misfeature that strtoul() parses
> "-3" successfully and returns ULONG_MAX-2, which was most certainly
> not intended by the user.
Agree; it is a misfeature. In my API a2i(), when the type passed in the
first parameter is an unsigned type, negative values are rejected.
I wonder if there's any legitimate user of that misfeature. I didn't
want to rule it out from a fundamental API just because I can't think of
a good use of it.
Maybe since we have people from many systems here, anyone who has even
seen a good use of strtoul(3) parsing negative values into an unsigned
type can comment. Maybe if we don't hear about it, we could consider it
useless and tighten it? Especially for an API that has explicit range
checks.
Would NetBSD be open to changing the implementation of strtou(3) to
reject negative input?
> > 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.
>
> Ouch. Yet another problem with strtoul(). That would be worth another
> error code in the specification of strtou().
No, in strtou(3) I think that should be part of ERANGE. If I specify
min=0, I really want -1 to trigger an ERANGE, just like 1 would trigger
an ERANGE if I pass min=2.
But yes, another issue in the list of "strtol(3) is really broken,
really.". :-)
> > I think this is not one case where you want silent saturation. You're
> > indeed doing range checks [0, UINT16_MAX].
>
> Yes, in this particular case, the ability to pass an arbitrary min and max
> to strtou() is a practical feature.
:)
>
> 2) nproc.c, omp_init.c
>
> > > gnulib/lib/nproc.c:402
> >
> > 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-{
> >
> > ...
> >
> > 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 = NULL;
> > lib/nproc.c:402: unsigned long int value = strtoul (threads, &endptr, 10);
> > lib/nproc.c-403-
> > lib/nproc.c-404- if (endptr != NULL)
> > lib/nproc.c-405- {
> > lib/nproc.c-406- while (*endptr != '\0' && c_isspace (*endptr))
> > lib/nproc.c-407- endptr++;
> > lib/nproc.c-408- if (*endptr == '\0')
> > lib/nproc.c-409- return value;
> > lib/nproc.c-410- /* Also accept the first value in a nesting level,
> > lib/nproc.c-411- since we can't determine the nesting level from env vars. */
> > lib/nproc.c-412- else if (*endptr == ',')
> > lib/nproc.c-413- return value;
> > lib/nproc.c-414- }
> > lib/nproc.c-415- }
> >
> > First of all, the endptr!=NULL test seems misplaced.
>
> I wasn't sure that all implementations store an endptr in all cases.
One of the many issues with strtol(3) to note in the list. This one is
rather obscure, because different platforms do different things. The
conclusion we reached when we discussed it back then is: never rely on
strtol(3) reporting EINVAL. If you've reached that point, you're really
deep into UB. Check the base before the actual call.
> > This could be something like this (fixing the bugs reported above):
> >
> > char *end;
> > u_long value;
> >
> > value = strtou_noneg(threads, &end, 10, 0, ULONG_MAX, NULL);
> > if (end != threads) {
> > end += strspn(end, " \t\n");
> > if (streq(end, "")
> > return value;
> > if (strprefix(end, ","))
> > return value;
> > }
> >
> >
> > This is one case where you seem to silently ignore saturation.
>
> This is on purpose. A CPU never has more than 1000 processors. Therefore
> all values > 1000 should be treated the same way, whether they are
> <= ULONG_MAX or > ULONG_MAX. Clamping to the number of actually available
> processors occurs later in the code.
I guess you refer to the MIN() calls within num_processors(), right?
Why do those clampings not result in diagnostics? Couldn't you
calculate the limits before parsing the actual number, and so use them
to perform the range checks during the strtou(3) call?
> 3) msgfmt.c
>
> > > gettext/gettext-tools/src/msgfmt.c:287
> >
> > gettext-tools/src/msgfmt.c-286- char *endp;
> > gettext-tools/src/msgfmt.c:287: size_t new_align = strtoul (optarg, &endp, 0);
> > gettext-tools/src/msgfmt.c-288-
> > gettext-tools/src/msgfmt.c-289- if (endp != optarg)
> > gettext-tools/src/msgfmt.c-290- alignment = new_align;
> >
> > This code will misbehave badly on platforms where size_t is narrower
> > than u_long.
>
> Such platforms (namely Windows 3.1) are not among the portability targets
> of this code.
Ok. Still, strtou(3) would make that a non-issue.
> > You also don't reject negative numbers, which I expect to be a bug,
> > connected with the one from above.
>
> That could be a problem. Fortunately, the documentation makes it clear
> that the argument is an "alignment", and reasonable users will therefore
> only pass the values 1, 2, 4, 8, 16.
Nice, then a call to strtou(3) would have the limits 1 and 16, which
would make it more robust. No need to actively ignore ERANGE.
> > Why don't you have a diagnostic message for invalid input?
>
> Because this option is meant for users who know what an "alignment" is.
But still, you don't need to actively ignore ERANGE, especially when it
results in more complex code than actually checking. Once the API gives
you the check for free, you should probably use it.
In this case, I think I'd do
int status;
size_t new_align;
new_align = strtou(optarg, NULL, 0, 1, 16, &status);
if (status == 0)
alignment = new_align;
Don't you think this is more robust and just as simple? I think it is a
'd' case because strtoul(3) makes it difficult for you to do the check,
not because you actively want to not check.
> 4) msgl-check.c
>
> > > gettext/gettext-tools/src/msgl-check.c:379
> >
> > gettext-tools/src/msgl-check.c-374- while (*nplurals != '\0' && c_isspace ((unsigned char) *nplurals))
> > gettext-tools/src/msgl-check.c-375- ++nplurals;
> > gettext-tools/src/msgl-check.c-376- endp = nplurals;
> > gettext-tools/src/msgl-check.c-377- nplurals_value = 0;
> > gettext-tools/src/msgl-check.c-378- if (*nplurals >= '0' && *nplurals <= '9')
> > gettext-tools/src/msgl-check.c:379: nplurals_value = strtoul (nplurals, (char **) &endp, 10);
> > gettext-tools/src/msgl-check.c-380- if (nplurals == endp)
> > gettext-tools/src/msgl-check.c-381- {
> > gettext-tools/src/msgl-check.c-382- const char *msg = _("invalid nplurals value");
> > gettext-tools/src/msgl-check.c-383- char *help = plural_help (nullentry);
> > gettext-tools/src/msgl-check.c-384-
> > gettext-tools/src/msgl-check.c-385- if (help != NULL)
> > gettext-tools/src/msgl-check.c-386- {
> > gettext-tools/src/msgl-check.c-387- char *msgext = xasprintf ("%s\n%s", msg, help);
> > gettext-tools/src/msgl-check.c-388- xeh->xerror (CAT_SEVERITY_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_SEVERITY_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- }
>
> > On the other hand, I also wonder why you don't diagnose invalid input.
> > Why is -10 an "invalid nplurals value"
>
> "-10" designates a negative number. For the number of plural forms, that's
> invalid.
>
> > but ULONG_MAX+10 is a valid (albeit clamped) one?
>
> Again, as mentioned above, values like 1000 and 10000000000000000 should
> be treated the same way. Since ULONG_MAX+10 gets clamped to ULONG_MAX, that's
> just fine.
I see in line 433 a check
if (min_nplurals < nplurals_value)
And in line 449 a check
else if (max_nplurals > nplurals_value)
nplurals_value was parsed via strtoul(3) in line 379.
And min_plurals and max_plurals were assigned in lines 293 and lines 298.
Which makes me wonder: why don't you move the tests from lines 4** into
the strtou(3) call?
That is, why not call this?
nplurals_value = strtou(nplurals, NULL, 10, min_nplurals, max_nplurals, &status);
And then have the ERANGE handling there?
> 5) read-stringtable.c
>
> > > gettext/gettext-tools/src/read-stringtable.c:561
> >
> > 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) >= 6 && memcmp (line, "File: ", 6) == 0
> > gettext-tools/src/read-stringtable.c-559- && (last_colon = strrchr (line + 6, ':')) != NULL
> > gettext-tools/src/read-stringtable.c-560- && *(last_colon + 1) != '\0'
> > gettext-tools/src/read-stringtable.c:561: && (number = strtoul (last_colon + 1, &endp, 10), *endp == '\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 = '\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_comment (catr, line);
> > gettext-tools/src/read-stringtable.c-569- }
> >
> > You're forgetting about negative numbers?
>
> Indeed, I didn't realize that strtoul() would accept "-3" as valid. As said
> above, strtou() can improve on it, by reserving an error code for it.
Yeah, if I had designed it, it would have ERANGE for those cases.
Maybe we're in time to reform it. I'll first open a NetBSD bug, and see
if we can convince them.
> > How about huge values?
>
> In this code, we assume that line numbers are <= ULONG_MAX. Not an
> unreasonable assumption. I have yet to see source code files that are
> 4 GiB large...
>
> > But again, I wonder why you don't do range checks.
>
> Range checks are not important here: The line numbers are not used as
> indices; they are merely reproduced in the output.
They're not important, but if the API gives you a range check for free,
I think you should take it, and consider a line number of ULONG_MAX + 1
to be an error. It will make the code more robust, I think.
> > > If you don't want to do that, I can only repeat what I said in the previous
> > > mail: The proposal *does not achieve the goal* of avoiding the most common
> > > programmer mistakes. For a robust API, the success test should *only* involve
> > > testing the returned 'status', nothing else.
> >
> > Let's discuss this after your responses to the above.
>
> In the cases 1), 2), 4), use-case d. was chosen on purpose.
Case 1 was not d. Case 1 does check the range. ?
In cases 2 and 4, I think you'd benefit from refactoring the code to use
the limits in the strtou(3) call. Especially in case 4 where you
already know the limits.
Have a lovely night!
Alex
--
<https://www.alejandro-colomar.es/>
Attachment:
signature.asc
Description: PGP signature