Re: Incorrect usage of strtol, atoi for non-numeric junk inputs

Thanks for the discussion.

At Tue, 13 Jul 2021 09:28:30 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in
> On Fri, Jul 09, 2021 at 04:50:28PM +0900, Kyotaro Horiguchi wrote:
> > At Fri, 9 Jul 2021 10:29:07 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in
> >> Er, wait. We've actually allowed negative values for pg_ctl
> >> --timeout or the subcommand kill!?
> >
> > --timeout accepts values less than 1, which values cause the command
> > ends with the timeout error before checking for the ending state. Not
> > destructive but useless as a behavior. We have two choices here. One
> > is simply inhibiting zero or negative timeouts, and another is
> > allowing zero as timeout and giving it the same meaning to
> > --no-wait. The former is strictly right behavior but the latter is
> > casual and convenient. I took the former way in this version.
>
> Yeah, that's not useful.

^^; Ok, I'm fine with taking the second way. Changed it.

"-t 0" means "-W" in the attached and that behavior is described in
the doc part. The environment variable PGCTLTIMEOUT accepts the same
range of values. I added a warning that notifies that -t 0 overrides
explicit -w .

> $ pg_ctl -w -t 0 start
> pg_ctl: WARNING: -w is ignored because timeout is set to 0
> server starting

> >> This one in pg_upgrade is incomplete. Perhaps the missing comment
> >> should tell that negative job values are checked later on?
> >
> > Zero or negative job numbers mean non-parallel mode and don't lead to
> > an error. If we don't need to preserve that behavior (I personally
> > don't think it is ether useful and/or breaks someone's existing
> > usage.), it is sensible to do range-check here.
>
> Hmm. It would be good to preserve some compatibility here, but I can
> see more benefits in being consistent between all the tools, and make
> people aware that such commands are not generated more carefully.

Ageed.

> > case 'j':
> > - opts.jobs = atoi(optarg);
> > - if (opts.jobs < 1)
> > + errno = 0;
> > + opts.jobs = strtoint(optarg, &endptr, 10);
> > + if (*endptr || errno == ERANGE || opts.jobs < 1)
> > {
> > pg_log_error("number of parallel jobs must be at least 1");
> > exit(1);
>
> specifying a value that triggers ERANGE could be confusing for values
> higher than INT_MAX with pg_amcheck --jobs:
> $ pg_amcheck --jobs=4000000000
> pg_amcheck: error: number of parallel jobs must be at least 1
> I think that this should be reworded as "invalid number of parallel
> jobs \"$s\"", or "number of parallel jobs must be in range %d..%d".
> Perhaps we could have a combination of both? Using the first message
> is useful with junk, non-numeric values or trailing characters. The
> second is useful to make users aware that the value is numeric, but
> not quite right.

Yeah, I noticed that but ignored as a kind of impossible, or
something-needless-to-say:p

> "invalid number of parallel jobs \"$s\""
> "number of parallel jobs must be in range %d..%d"

The resulting combined message looks like this:

> "number of parallel jobs must be an integer in range 1..2147483647"

I don't think it's not great that the message explicitly suggests a
limit like INT_MAX, which is far above the practical limit. So, (even
though I avoided to do that but) in the attached, I changed my mind to
split most of the errors into two messages to avoid suggesting such
impractical limits.

$ pg_amcheck -j -1
$ pg_amcheck -j 1x
$ pg_amcheck -j 10000000000000x
pg_amcheck: error: number of parallel jobs must be an integer greater than zero: "...."
$ pg_amcheck -j 10000000000000
pg_amcheck: error: number of parallel jobs out of range: "10000000000000"

If you feel it's too-much or pointless to split that way, I'll happy
to change it the "%d..%d" form.

Still I used the "%d..%d" notation for some parameters that has a
finite range by design. They look like the following:
(%d..%d doesn't work well for real numbers.)

"sampling rate must be an real number between 0.0 and 1.0: \"%s\""

I'm not sure what to do for numWorkers of pg_dump/restore. The limit
for numWorkers are lowered on Windows to quite low value, which would
be worth showing, but otherwise the limit is INT_MAX. The attached
makes pg_dump respond to -j 100 with the following error message which
doesn't suggest the finite limit 64 on Windows.

$ pg_dump -j 100
pg_dump: error: number of parallel jobs out of range: "100"

> > @@ -587,8 +602,10 @@ main(int argc, char **argv)
> >
> > case 8:
> > have_extra_float_digits = true;
> > - extra_float_digits = atoi(optarg);
> > - if (extra_float_digits < -15 || extra_float_digits > 3)
> > + errno = 0;
> > + extra_float_digits = strtoint(optarg, &endptr, 10);
> > + if (*endptr || errno == ERANGE ||
> > + extra_float_digits < -15 || extra_float_digits > 3)
> > {
> > pg_log_error("extra_float_digits must be in range -15..3");
> > exit_nicely(1);
>
> Should we take this occasion to reduce the burden of translators and
> reword that as "%s must be in range %d..%d"? That could be a separate
> patch.

The first %s is not always an invariable symbol name so it could
result in concatenating several phrases into one, like this.

pg_log..("%s must be in range %s..%s", _("compression level"), "0", "9"))

It is translatable at least into Japanese but I'm not sure about other
languages.

In the attached, most of the messages are not in this shape since I
avoided to suggest substantially infinite limits, thus this doesn't
fully work. I'll consider it if the current shape is found to be
unacceptable. In that case I'll use the option names in the messages
instead of the natural names.

> > case 'p':
> > - if ((old_cluster.port = atoi(optarg)) <= 0)
> > - pg_fatal("invalid old port number\n");
> > + errno = 0;
> > + if ((old_cluster.port = strtoint(optarg, &endptr, 10)) <= 0 ||
> > + *endptr || errno == ERANGE)
> > + pg_fatal("invalid old port number %s\n", optarg);
> > break;
>
> You may want to use columns here, or specify the port range:
> "invalid old port number: %s" or "old port number must be in range
> %d..%d".

I'm not sure whether the colons(?) are required, since pg_receivewal
currently complains as "invalid port number \"%s\"" but I agree that
it would be better if we had one.

By the way, in the attached version, the message is split into the
following combination. ("an integer" might be useless..)

pg_fatal("old port number must be an integer greater than zero: \"%s\"\n",
pg_fatal("old port number out of range: \"%s\"\n", optarg);

As the result.

> > + if (*endptr || errno == ERANGE || concurrentCons <= 0)
> > {
> > - pg_log_error("number of parallel jobs must be at least 1");
> > + pg_log_error("number of parallel jobs must be at least 1: %s", optarg);
> > exit(1);
> > }
>
> This one is also confusing with optorg > INT_MAX.

The attached version says just "out of range" in that case.

- Is it worth avoiding suggesting a virtually infinite upper limit by
splitting out "out of range" from an error message?

- If not, I'll use the single message "xxx must be in range
1..2147483647" or "xxx must be an integer in range 1..2147483647".

Do you think we need the parameter type "an integer" there?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center