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

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.

>> 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.

> 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.

> --- a/src/bin/pg_checksums/pg_checksums.c
> case 'f':
> - if (atoi(optarg) == 0)
> + errno = 0;
> + if (strtoint(optarg, &endptr, 10) == 0
> + || *endptr || errno == ERANGE)
> {
> pg_log_error("invalid filenode specification, must be numeric: %s", optarg);
> exit(1);

The confusion is equal here with pg_checksums -f:
$ ./pg_checksums --f 4000000000
pg_checksums: error: invalid filenode specification, must be numeric: 400000000
We could say "invalid file specification: \"%s\"". Another idea to be
crystal-clear about the range requirements is to use that:
"file specification must be in range %d..%d"

> @@ -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.

> 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".

> case 'P':
> - if ((new_cluster.port = atoi(optarg)) <= 0)
> - pg_fatal("invalid new port number\n");
> + errno = 0;
> + if ((new_cluster.port = strtoint(optarg, &endptr, 10)) <= 0 ||
> + *endptr || errno == ERANGE)
> + pg_fatal("invalid new port number %s\n", optarg);
> break;

Ditto.

> + 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.

> + concurrentCons = strtoint(optarg, &endptr, 10);
> + if (*endptr || errno == ERANGE || concurrentCons <= 0)
> {
> pg_log_error("number of parallel jobs must be at least 1");
> exit(1);
> }
> break;

And ditto for all the ones of vacuumdb.
--
Michael