Hi Bruno,
On Thu, Mar 20, 2025 at 03:26:46PM +0100, Bruno Haible wrote:
> > > 2) nproc.c, omp_init.c
> > 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?
>
> The essential code is like this:
>
> unsigned long int omp_env_limit = ULONG_MAX;
>
> if (query == NPROC_CURRENT_OVERRIDABLE)
> {
> omp_env_limit = parse_omp_threads (getenv ("OMP_THREAD_LIMIT"));
> if (! omp_env_limit)
> omp_env_limit = ULONG_MAX;
> ...
> query = NPROC_CURRENT;
> }
> unsigned long nprocs = num_processors_ignoring_omp (query);
> return MIN (nprocs, omp_env_limit);
>
> and num_processors_ignoring_omp (query) is expensive to compute, since
> it makes system calls. For this reason, we do the parsing of environment
> variables first; this gives us the opportunity to optimize away the
> system calls in particular circumstances.[1]
Okay, I guess for such an exceptional optimization, you could live with
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;
}
since 'end != threads' means "something was parsed".
It deviates from testing 'status' for error handling, but you're
optimizing, so it's expected that you'll not do the obvious thing. That
keeps usage for most people reasonable. And remember that I have seen
zero cases of such calls in existing code calling strtoi(3)/strtou(3).
> > > 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;
> > > > ...
> > > > 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?
> > ...
> > strtoul(3) makes it difficult for you to do the check,
> > not because you actively want to not check.
>
> Yes and no. Yes, it would be nice to have a range check at essentially
> zero cost.
Good.
> No, clamping is not the right behaviour: an alignment of 5 or
> 17 should be rejected, not mapped to something valid.
I'm not clamping there; I'm rejecting the value. See that I'm only
executing 'alignment = ...' under 'if (status == 0)'.
Or, rather than rejecting it, I'm ignoring it, but that was already
being done for invalid input (e.g., -1). What I've done is to treat 17
just like -1.
Compare the current code:
gettext-tools/src/msgfmt.c-284- case 'a':
gettext-tools/src/msgfmt.c-285- {
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;
gettext-tools/src/msgfmt.c-291- }
gettext-tools/src/msgfmt.c-292- break;
with my suggested code:
case 'a':
{
int status;
size_t new_align;
new_align = strtou(optarg, NULL, 0, 1, 16, &status);
if (status == 0)
alignment = new_align;
}
break;
> > > 4) msgl-check.c
> > >
> > > > > gettext/gettext-tools/src/msgl-check.c:379
> >
> > 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?
>
> For two reasons:
>
> - The program's logic is to parse simple things first and complicated things
> afterwards. The user is likely to make a mistake in the simple things with
> a lower probability than in the complicated things. When there is a
> mismatch, the likely culprit is a mistake in the complicated things; the
> diagnostics need to be directed at this scenario.
>
> - It feels strange to move complicated validation logic into a library
> function.
Then it's your choice. You can again test (nplurals == end) to make
sure that "something was parsed", if you want to delay the actual range
checks.
BTW, manual checks like n>max are brittle. Consider a user writing a
ULONG_MAX + 1 in the string, which gets clamped into ULONG_MAX in
strtoul(3), and if for some reason your max value is ULONG_MAX, n will
silently be understood as ULONG_MAX. I've seen that bug in groff in
several places, and don't remember if in other places.
In general, not checking ERANGE right after a strtoul(3) call is prone
to this bug. The fact that strtou(3) forces you to specify a range is a
good reminder that you should not delay your checks unless you know what
you're doing.
> In summary, while the built-in range checks are welcome for things like
> port numbers (0..65536), in this case it's better to keep the program's logic
> explicit and situation-aware.
>
> > 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.
>
> No. As the author of that code, I disagree.
>
> Bruno
>
> [1] https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=205078f891d87e0e966ad8616fdf0306437f0370
Have a lovely day!
Alex
--
<https://www.alejandro-colomar.es/>
Attachment:
signature.asc
Description: PGP signature