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

Thank you for the comments.

At Fri, 9 Jul 2021 10:29:07 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in
> On Thu, Jul 08, 2021 at 05:30:23PM +0900, Kyotaro Horiguchi wrote:
> > [1] is trying to expose pg_strtoint16/32 to frontend, but I don't see
> > much point in doing that in conjunction with [2] or this thread. Since
> > the integral parameter values of pg-commands are in int, which the
> > exising function strtoint() is sufficient to read. So even [2] itself
> > doesn't need to utilize [1].
>
> It sounds sensible from here to just use strtoint(), some strtol(),
> son strtod() and call it a day as these are already available.

Thanks.

> > - wait_seconds = atoi(optarg);
> > + errno = 0;
> > + wait_seconds = strtoint(optarg, &endptr, 10);
> > + if (*endptr || errno == ERANGE || wait_seconds < 0)
> > + {
> > + pg_log_error("invalid timeout \"%s\"", optarg);
> > + exit(1);
> > + }
> > [ ... ]
> > - killproc = atol(argv[++optind]);
> > + errno = 0;
> > + killproc = strtol(argv[++optind], &endptr, 10);
> > + if (*endptr || errno == ERANGE || killproc < 0)
> > + {
> > + pg_log_error("invalid process ID \"%s\"", argv[optind]);
> > + exit(1);
> > + }
>
> Er, wait. We've actually allowed negative values for pg_ctl
> --timeout or the subcommand kill!?

For killproc, leading minus sign suggests that it is an command line
option. Since pg_ctl doesn't have such an option, that values is
recognized as invalid *options*, even with the additional check. The
additional check is useless in that sense. My intension is just to
make the restriction explicit so I won't feel sad even if it should be
removed.

$ pg_ctl kill HUP -12345
cg_ctl: infalid option -- '1'

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

$ pg_ctl -w -t 0 start
pg_ctl: error: invalid timeout value "0", use --no-wait to finish without waiting

The same message is shown for non-decimal values but that would not matters.

> > case 'j':
> > - user_opts.jobs = atoi(optarg);
> > + errno = 0;
> > + user_opts.jobs = strtoint(optarg, &endptr, 10);
> > + /**/
> > + if (*endptr || errno == ERANGE)
> > + pg_fatal("invalid number of jobs %s\n", optarg);
> > +
> > break;
>
> 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.

I noticed that I overlooked PGCTLTIMEOUT.

The attached is:

- disallowed less-than-one numbers as jobs in pg_upgrade.
- disallowed less-than-one timeout in pg_ctl
- Use strtoint for PGCTLTIMEOUT in pg_ctl is

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center