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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
Cc: alvherre(at)alvh(dot)no-ip(dot)org, bharath(dot)rupireddyforpostgres(at)gmail(dot)com, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Incorrect usage of strtol, atoi for non-numeric junk inputs
Date: 2021-07-21 11:49:45
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Jul 21, 2021 at 05:02:29PM +0900, Kyotaro Horiguchi wrote:
> The difference is your suggestion is making the function output the
> message within. I guess that the reason for the original proposal is
> different style of message is required in several places.

That's one step toward having a maximum number of frontend tools to
use the central logging APIs of src/common/.

> 1. Some "bare" options (that is not preceded by a hyphen option) like
> PID of pg_ctl kill doesn't fit the format. \pset parameters of
> pg_ctl is the same.

Yep. I was reviewing this one, but I have finished by removing it.
The argument 2 just below also came into my mind.

> 2. pg_ctl, pg_upgrade use their own error reporting mechanisms.

Yeah, for this reason I don't think that it is a good idea to switch
those areas to use the parsing of option_utils.c. Perhaps we should
consider switching pg_upgrade to have a better logging infra, but
there are also reasons behind what we have now. pg_ctl is out of
scope as it needs to cover WIN32 event logging.

> 3. parameters that take real numbers doesn't fit the scheme specifying
> range borders. For example boundary values may or may not be included
> in the range.

This concerns only pgbench, which I'd be fine to let as-is.

> 4. Most of the errors are PG_LOG_ERROR, but a few ones are

I would take it that pgbench is inconsistent with the rest. Note that
pg_dump uses fatal(), but that's just a wrapper to pg_log_error().

> loglevel specifies the loglevel to use to emit error messages. If it
> is the newly added item PG_LOG_OMIT, the function never emits an error
> message. Addition to that, the return type is changed to an enum which
> indicates what trouble the given string has. The caller can print
> arbitrary error messages consulting the value. (killproc in pg_ctl.c)

I am not much a fan of that. If we do so, what's the point in having
a dependency to logging.c anyway in option_utils.c? This OMIT option
only exists to bypass the specific logging needs where this gets
added. That does not seem a design adapted to me in the long term,
neither am I a fan of specific error codes for a code path that's just
going to be used to parse command options.

> I added two more similar functions option_parse_long/double. The
> former is a clone of _int. The latter doesn't perform explicit range
> checks for the reason described above.

These have a limited impact, so I would limit things to int32 for

> Maybe we need to make pg_upgrade use the common-logging features
> instead of its own, but it is not included in this patch.

Maybe. That would be good in the long term, though its case is very

> The attached patch needs more polish but should be enough to tell what
> I have in my mind.

This breaks some of the TAP tests of pgbench and pg_dump, at short

The checks for the port value in pg_receivewal and pg_recvlogical is
strange to have. We don't care about that in any other tools.

The number of checks for --jobs and workers could be made more
consistent across the board, but I have let that out for now.

Hacking on that, I am finishing with the attached. It is less
ambitious, still very useful as it removes a dozen of custom error
messages in favor of the two ones introduced in option_utils.c. On
top of that this reduces a bit the code:
15 files changed, 156 insertions(+), 169 deletions(-)

What do you think?

Attachment Content-Type Size
v4-0001-Introduce-and-use-routine-for-parsing-of-int32-op.patch text/x-diff 23.7 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Ronan Dunklau 2021-07-21 12:28:30 Re: ORDER BY pushdowns seem broken in postgres_fdw
Previous Message John Naylor 2021-07-21 11:44:06 Re: add 'noError' to euc_tw_and_big5.c