Re: Incorrect usage of strtol, atoi for non-numeric junk inputs - Mailing list pgsql-hackers
| From | Kyotaro Horiguchi |
|---|---|
| Subject | Re: Incorrect usage of strtol, atoi for non-numeric junk inputs |
| Date | |
| Msg-id | 20210721.170229.1864410878946243716.horikyota.ntt@gmail.com Whole thread Raw |
| In response to | Incorrect usage of strtol, atoi for non-numeric junk inputs (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>) |
| List | pgsql-hackers |
At Thu, 15 Jul 2021 16:19:07 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> On Wed, Jul 14, 2021 at 11:02:47AM -0400, Alvaro Herrera wrote:
> > No, this doesn't work. When the first word is something that is
> > not to be translated (a literal parameter name), let's use a %s (but of
> > course the values must be taken out of the phrase too). But when it is
> > a translatable phrase, it must be present a full phrase, no
> > substitution:
> >
> > pg_log_error("%s must be in range %s..%s", "extra_float_digits", "-15", "3");
> > pg_log..("compression level must be in range %s..%s", "0", "9"))
> >
> > I think the purity test is whether you want to use a _() around the
> > argument, then you have to include it into the original message.
>
> After thinking about that, it seems to me that we don't have that much
> context to lose once we build those error messages to show the option
> name of the command. And the patch, as proposed, finishes with the
Agreed.
> same error message patterns all over the place, which would be a
> recipe for more inconsistencies in the future. I think that we should
> centralize all that, say with a small-ish routine in a new file called
> src/fe_utils/option_parsing.c that uses strtoint() as follows:
> bool option_parse_int(const char *optarg,
> const char *optname,
> int min_range,
> int max_range,
> int *result);
>
> Then this routine may print two types of errors through
> pg_log_error():
> - Incorrect range, using min_range/max_range.
> - Junk input.
> The boolean status is here to let the caller do any custom exit()
> actions he wishes if there one of those two failures. pg_dump has
> some of that with exit_nicely(), for one.
It is substantially the same suggestion with [1] in the original
thread. The original proposal in the old thread was
> bool pg_strtoint64_range(arg, &result, min, max, &error_message)
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.
Actually there are several irregulars.
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.
2. pg_ctl, pg_upgrade use their own error reporting mechanisms.
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.
4. Most of the errors are PG_LOG_ERROR, but a few ones are
PG_LOG_FATAL.
That being said, most of the caller sites are satisfied by
fixed-formed messages.
So I changed the interface as the following to deal with the above issues:
+extern optparse_result option_parse_int(int loglevel,
+ const char *optarg, const char *optname,
+ int min_range, int max_range,
+ int *result);
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)
Other points:
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.
Maybe we need to make pg_upgraded use the common-logging features
instead of its own, but it is not included in this patch.
pgbench's -L option accepts out-of-range values for internal
variable. As the added comment says, we can limit the value with the
large exact number but I limited it to 3600s since I can't imagine
people needs larger latency limit than that.
Similarly to the above, -R option can take for example 1E-300, which
leads to int64 overflow later. Similar to -L, I don't think people
don't need less than 1E-5 or so as this parameter.
The attached patch needs more polish but should be enough to tell what
I have in my mind.
regards.
1: https://www.postgresql.org/message-id/CAKJS1f94kkuB_53LcEf0HF%2BuxMiTCk5FtLx9gSsXcCByUKMz1g%40mail.gmail.com
--
Kyotaro Horiguchi
NTT Open Source Software Center
From aaf81ac0340e9df3b74e18c1492e5984c0412fb5 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 8 Jul 2021 15:08:01 +0900
Subject: [PATCH v3 1/3] Be strict in numeric parameters on command line
Some numeric command line parameters are tolerant of valid values
followed by garbage like "123xyz". Be strict to reject such invalid
values. Do the same for psql meta command parameters.
---
src/bin/pg_amcheck/pg_amcheck.c | 9 +-
src/bin/pg_basebackup/pg_basebackup.c | 17 ++-
src/bin/pg_basebackup/pg_receivewal.c | 25 ++--
src/bin/pg_basebackup/pg_recvlogical.c | 30 ++---
src/bin/pg_checksums/Makefile | 4 +-
src/bin/pg_checksums/pg_checksums.c | 9 +-
src/bin/pg_ctl/Makefile | 4 +-
src/bin/pg_ctl/pg_ctl.c | 39 ++++++-
src/bin/pg_dump/pg_dump.c | 44 ++++---
src/bin/pg_dump/pg_restore.c | 29 ++---
src/bin/pg_upgrade/option.c | 24 +++-
src/bin/pgbench/pgbench.c | 119 ++++++++++---------
src/bin/psql/command.c | 63 ++++++++--
src/bin/scripts/reindexdb.c | 9 +-
src/bin/scripts/vacuumdb.c | 31 ++---
src/fe_utils/option_utils.c | 152 +++++++++++++++++++++++++
src/include/common/logging.h | 9 ++
src/include/fe_utils/option_utils.h | 23 ++++
18 files changed, 457 insertions(+), 183 deletions(-)
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 4bde16fb4b..538436d4a5 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -12,11 +12,13 @@
*/
#include "postgres_fe.h"
+#include <limits.h>
#include <time.h>
#include "catalog/pg_am_d.h"
#include "catalog/pg_namespace_d.h"
#include "common/logging.h"
+#include "common/string.h"
#include "common/username.h"
#include "fe_utils/cancel.h"
#include "fe_utils/option_utils.h"
@@ -326,12 +328,9 @@ main(int argc, char *argv[])
append_btree_pattern(&opts.exclude, optarg, encoding);
break;
case 'j':
- opts.jobs = atoi(optarg);
- if (opts.jobs < 1)
- {
- pg_log_error("number of parallel jobs must be at least 1");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-j/--jobs",
+ 1, INT_MAX, &opts.jobs))
exit(1);
- }
break;
case 'p':
port = pg_strdup(optarg);
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8bb0acf498..42a65ffbc3 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -15,6 +15,7 @@
#include <unistd.h>
#include <dirent.h>
+#include <limits.h>
#include <sys/stat.h>
#include <sys/wait.h>
#include <signal.h>
@@ -31,6 +32,7 @@
#include "common/file_utils.h"
#include "common/logging.h"
#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/recovery_gen.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
@@ -2371,12 +2373,9 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
- {
- pg_log_error("invalid compression level \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-Z/--compress",
+ 0, 9, &compresslevel))
exit(1);
- }
break;
case 'c':
if (pg_strcasecmp(optarg, "fast") == 0)
@@ -2409,12 +2408,10 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
- {
- pg_log_error("invalid status interval \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg,
+ "-s/--status-interval",
+ 0, INT_MAX, &standby_message_timeout))
exit(1);
- }
break;
case 'v':
verbose++;
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index c1334fad35..1f976ad3eb 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -15,6 +15,7 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <limits.h>
#include <signal.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -22,6 +23,8 @@
#include "access/xlog_internal.h"
#include "common/file_perm.h"
#include "common/logging.h"
+#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "receivelog.h"
@@ -532,11 +535,10 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
- {
- pg_log_error("invalid port number \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-p/--port",
+ 1, 65535, NULL))
exit(1);
- }
+
dbport = pg_strdup(optarg);
break;
case 'U':
@@ -549,12 +551,10 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
- {
- pg_log_error("invalid status interval \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg,
+ "-s/--status-interval",
+ 0, INT_MAX, &standby_message_timeout))
exit(1);
- }
break;
case 'S':
replication_slot = pg_strdup(optarg);
@@ -574,12 +574,9 @@ main(int argc, char **argv)
verbose++;
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
- {
- pg_log_error("invalid compression level \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-Z/--compress",
+ 0, 9, &compresslevel))
exit(1);
- }
break;
/* action */
case 1:
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 76bd153fac..44622c002b 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -13,6 +13,7 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <limits.h>
#include <sys/stat.h>
#include <unistd.h>
#ifdef HAVE_SYS_SELECT_H
@@ -23,6 +24,8 @@
#include "common/fe_memutils.h"
#include "common/file_perm.h"
#include "common/logging.h"
+#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "libpq/pqsignal.h"
@@ -739,12 +742,12 @@ main(int argc, char **argv)
outfile = pg_strdup(optarg);
break;
case 'F':
- fsync_interval = atoi(optarg) * 1000;
- if (fsync_interval < 0)
- {
- pg_log_error("invalid fsync interval \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg,
+ "-F/--fsync-interval",
+ 0, INT_MAX / 1000, &fsync_interval))
exit(1);
- }
+ /* convert to milliseconds */
+ fsync_interval *= 1000;
break;
case 'n':
noloop = 1;
@@ -763,11 +766,9 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
- {
- pg_log_error("invalid port number \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-p/--port",
+ 0, 65535, NULL))
exit(1);
- }
dbport = pg_strdup(optarg);
break;
case 'U':
@@ -820,12 +821,13 @@ main(int argc, char **argv)
plugin = pg_strdup(optarg);
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
- {
- pg_log_error("invalid status interval \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg,
+ "-s/--status-interval",
+ 0, INT_MAX / 1000,
+ &standby_message_timeout))
exit(1);
- }
+ /* convert to milliseconds */
+ standby_message_timeout *= 1000;
break;
case 'S':
replication_slot = pg_strdup(optarg);
diff --git a/src/bin/pg_checksums/Makefile b/src/bin/pg_checksums/Makefile
index ba62406105..a22cf107f9 100644
--- a/src/bin/pg_checksums/Makefile
+++ b/src/bin/pg_checksums/Makefile
@@ -15,13 +15,15 @@ subdir = src/bin/pg_checksums
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils
+
OBJS = \
$(WIN32RES) \
pg_checksums.o
all: pg_checksums
-pg_checksums: $(OBJS) | submake-libpgport
+pg_checksums: $(OBJS) | submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
install: all installdirs
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 3c326906e2..d62df9747a 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -15,6 +15,7 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <limits.h>
#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -24,6 +25,8 @@
#include "common/file_perm.h"
#include "common/file_utils.h"
#include "common/logging.h"
+#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "pg_getopt.h"
#include "storage/bufpage.h"
@@ -518,11 +521,9 @@ main(int argc, char *argv[])
mode = PG_MODE_ENABLE;
break;
case 'f':
- if (atoi(optarg) == 0)
- {
- pg_log_error("invalid filenode specification, must be numeric: %s", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-f/--filenode",
+ 1, INT_MAX, NULL))
exit(1);
- }
only_filenode = pstrdup(optarg);
break;
case 'N':
diff --git a/src/bin/pg_ctl/Makefile b/src/bin/pg_ctl/Makefile
index 5d5f5372a3..ed03d6dc7d 100644
--- a/src/bin/pg_ctl/Makefile
+++ b/src/bin/pg_ctl/Makefile
@@ -24,13 +24,15 @@ LDFLAGS_INTERNAL += $(libpq_pgport)
SUBMAKE_LIBPQ := submake-libpq
endif
+LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils
+
OBJS = \
$(WIN32RES) \
pg_ctl.o
all: pg_ctl
-pg_ctl: $(OBJS) | submake-libpgport $(SUBMAKE_LIBPQ)
+pg_ctl: $(OBJS) | submake-libpgport $(SUBMAKE_LIBPQ) submake-libpgfeutils
$(CC) $(CFLAGS) $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
install: all installdirs
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 7985da0a94..f7ccdcdc96 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -12,6 +12,7 @@
#include "postgres_fe.h"
#include <fcntl.h>
+#include <limits.h>
#include <signal.h>
#include <time.h>
#include <sys/stat.h>
@@ -28,6 +29,7 @@
#include "common/file_perm.h"
#include "common/logging.h"
#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "utils/pidfile.h"
@@ -76,6 +78,7 @@ typedef enum
#define WAITS_PER_SEC 10 /* should divide USEC_PER_SEC evenly */
+static bool do_wait_arg = false;
static bool do_wait = true;
static int wait_seconds = DEFAULT_WAIT;
static bool wait_seconds_arg = false;
@@ -2396,7 +2399,9 @@ main(int argc, char **argv)
#endif
break;
case 't':
- wait_seconds = atoi(optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-t/--timeout",
+ 0, INT_MAX, &wait_seconds))
+ exit(1);
wait_seconds_arg = true;
break;
case 'U':
@@ -2408,6 +2413,7 @@ main(int argc, char **argv)
break;
case 'w':
do_wait = true;
+ do_wait_arg = true;
break;
case 'W':
do_wait = false;
@@ -2459,7 +2465,28 @@ main(int argc, char **argv)
}
ctl_command = KILL_COMMAND;
set_sig(argv[++optind]);
- killproc = atol(argv[++optind]);
+ switch (option_parse_long(PG_LOG_OMIT, argv[++optind], "PID",
+ 0, LONG_MAX, &killproc))
+ {
+ case OPTPARSE_SUCCESS:
+ break;
+ case OPTPARSE_MALFORMED:
+ write_stderr(_("%s: PID must be an integer: \"%s\"\n"),
+ progname, argv[optind]);
+ exit(1);
+ break;
+ case OPTPARSE_TOOSMALL:
+ write_stderr(_("%s: PID must be a non-negative integer: %s\n"),
+ progname, argv[optind]);
+ exit(1);
+ break;
+ case OPTPARSE_TOOLARGE:
+ case OPTPARSE_OUTOFRANGE:
+ write_stderr(_("%s: PID out of range: %s\n"),
+ progname, argv[optind]);
+ exit(1);
+ break;
+ }
}
#ifdef WIN32
else if (strcmp(argv[optind], "register") == 0)
@@ -2514,6 +2541,14 @@ main(int argc, char **argv)
do_wait = false;
}
+ if (wait_seconds == 0 && do_wait)
+ {
+ /* Warn if user instructed to wait but we actually don't */
+ if (!silent_mode && do_wait_arg)
+ write_stderr(_("%s: WARNING: -w is ignored because timeout is set to 0\n"), progname);
+ do_wait = false;
+ }
+
if (pg_data)
{
snprintf(postopts_file, MAXPGPATH, "%s/postmaster.opts", pg_data);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 34b91bb226..41b360edc0 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -55,7 +55,9 @@
#include "catalog/pg_trigger_d.h"
#include "catalog/pg_type_d.h"
#include "common/connect.h"
+#include "common/string.h"
#include "dumputils.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "libpq/libpq-fs.h"
@@ -104,6 +106,17 @@ static Oid g_last_builtin_oid; /* value of the last builtin oid */
/* The specified names/patterns should to match at least one entity */
static int strict_names = 0;
+/*
+ * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually)
+ * parallel jobs because that's the maximum limit for the
+ * WaitForMultipleObjects() call.
+ */
+#ifndef WIN32
+#define MAX_NUM_WORKERS INT_MAX
+#else
+#define MAX_NUM_WORKERS MAXIMUM_WAIT_OBJECTS
+#endif
+
/*
* Object inclusion/exclusion lists
*
@@ -487,7 +500,9 @@ main(int argc, char **argv)
break;
case 'j': /* number of dump jobs */
- numWorkers = atoi(optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-j/--jobs",
+ 1, MAX_NUM_WORKERS, &numWorkers))
+ exit_nicely(1);
break;
case 'n': /* include schema(s) */
@@ -550,12 +565,9 @@ main(int argc, char **argv)
break;
case 'Z': /* Compression Level */
- compressLevel = atoi(optarg);
- if (compressLevel < 0 || compressLevel > 9)
- {
- pg_log_error("compression level must be in range 0..9");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-Z/--compress",
+ 0, 9, &compressLevel))
exit_nicely(1);
- }
break;
case 0:
@@ -588,12 +600,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)
- {
- pg_log_error("extra_float_digits must be in range -15..3");
+ if (option_parse_int(PG_LOG_ERROR, optarg,
+ "--extra-float-digits",
+ -15, 3, &extra_float_digits))
exit_nicely(1);
- }
break;
case 9: /* inserts */
@@ -720,18 +730,6 @@ main(int argc, char **argv)
if (!plainText)
dopt.outputCreateDB = 1;
- /*
- * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually)
- * parallel jobs because that's the maximum limit for the
- * WaitForMultipleObjects() call.
- */
- if (numWorkers <= 0
-#ifdef WIN32
- || numWorkers > MAXIMUM_WAIT_OBJECTS
-#endif
- )
- fatal("invalid number of parallel jobs");
-
/* Parallel backup only in the directory archive format so far */
if (archiveFormat != archDirectory && numWorkers > 1)
fatal("parallel backup only supported by the directory format");
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 589b4aed53..6f4447027a 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -39,6 +39,8 @@
*-------------------------------------------------------------------------
*/
#include "postgres_fe.h"
+#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include <ctype.h>
#ifdef HAVE_TERMIOS_H
@@ -52,6 +54,13 @@
static void usage(const char *progname);
+/* See comments in pg_dump.c */
+#ifndef WIN32
+#define MAX_NUM_WORKERS INT_MAX
+#else
+#define MAX_NUM_WORKERS MAXIMUM_WAIT_OBJECTS
+#endif
+
int
main(int argc, char **argv)
{
@@ -181,7 +190,9 @@ main(int argc, char **argv)
break;
case 'j': /* number of restore jobs */
- numWorkers = atoi(optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-j/--jobs",
+ 1, MAX_NUM_WORKERS, &numWorkers))
+ exit(1);
break;
case 'l': /* Dump the TOC summary */
@@ -344,22 +355,6 @@ main(int argc, char **argv)
exit_nicely(1);
}
- if (numWorkers <= 0)
- {
- pg_log_error("invalid number of parallel jobs");
- exit(1);
- }
-
- /* See comments in pg_dump.c */
-#ifdef WIN32
- if (numWorkers > MAXIMUM_WAIT_OBJECTS)
- {
- pg_log_error("maximum number of parallel jobs is %d",
- MAXIMUM_WAIT_OBJECTS);
- exit(1);
- }
-#endif
-
/* Can't do single-txn mode with multiple connections */
if (opts->single_txn && numWorkers > 1)
{
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 64bbda5650..a8cd23f7fa 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -9,12 +9,15 @@
#include "postgres_fe.h"
+#include <limits.h>
#include <time.h>
#ifdef WIN32
#include <io.h>
#endif
+#include "common/logging.h"
#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "pg_upgrade.h"
#include "utils/pidfile.h"
@@ -104,6 +107,8 @@ parseCommandLine(int argc, char *argv[])
while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rs:U:v",
long_options, &optindex)) != -1)
{
+ int p;
+
switch (option)
{
case 'b':
@@ -127,7 +132,10 @@ parseCommandLine(int argc, char *argv[])
break;
case 'j':
- user_opts.jobs = atoi(optarg);
+ if (option_parse_int(PG_LOG_OMIT, optarg, "-j/--jobs",
+ 0, INT_MAX, &user_opts.jobs))
+ pg_fatal("-j/--jobs must be an integer in range %d..%d: \"%s\"\n",
+ 0, INT_MAX, optarg);
break;
case 'k':
@@ -166,13 +174,19 @@ parseCommandLine(int argc, char *argv[])
* supported on all old/new versions (added in PG 9.2).
*/
case 'p':
- if ((old_cluster.port = atoi(optarg)) <= 0)
- pg_fatal("invalid old port number\n");
+ if (option_parse_int(PG_LOG_OMIT, optarg, "-p/--old-port",
+ 1, 65535, &p))
+ pg_fatal("old port number must be an integer in range %d..%d: \"%s\"\n",
+ 1, 65535, optarg);
+ old_cluster.port = p;
break;
case 'P':
- if ((new_cluster.port = atoi(optarg)) <= 0)
- pg_fatal("invalid new port number\n");
+ if (option_parse_int(PG_LOG_OMIT, optarg, "-P/--new-port",
+ 1, 65535, &p))
+ pg_fatal("new port number must be an integer in range %d..%d: \"%s\"\n",
+ 1, 65535, optarg);
+ new_cluster.port = p;
break;
case 'r':
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 364b5a2e47..faa0969d70 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -63,6 +63,7 @@
#include "common/username.h"
#include "fe_utils/cancel.h"
#include "fe_utils/conditional.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
@@ -871,7 +872,12 @@ invalid_syntax:
return false;
}
-/* convert string to double, detecting overflows/underflows */
+/*
+ * convert string to double, detecting overflows/underflows
+ *
+ * This is similar to option_parse_double but leave this function for
+ * performance reasons.
+ */
bool
strtodouble(const char *str, bool errorOK, double *dv)
{
@@ -5887,12 +5893,9 @@ main(int argc, char **argv)
break;
case 'c':
benchmarking_option_set = true;
- nclients = atoi(optarg);
- if (nclients <= 0)
- {
- pg_log_fatal("invalid number of clients: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-c/--client",
+ 1, INT_MAX, &nclients))
exit(1);
- }
#ifdef HAVE_GETRLIMIT
#ifdef RLIMIT_NOFILE /* most platforms use RLIMIT_NOFILE */
if (getrlimit(RLIMIT_NOFILE, &rlim) == -1)
@@ -5914,12 +5917,10 @@ main(int argc, char **argv)
break;
case 'j': /* jobs */
benchmarking_option_set = true;
- nthreads = atoi(optarg);
- if (nthreads <= 0)
- {
- pg_log_fatal("invalid number of threads: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-j/--jobs",
+ 0, INT_MAX, &nthreads))
exit(1);
- }
+
#ifndef ENABLE_THREAD_SAFETY
if (nthreads != 1)
{
@@ -5938,30 +5939,21 @@ main(int argc, char **argv)
break;
case 's':
scale_given = true;
- scale = atoi(optarg);
- if (scale <= 0)
- {
- pg_log_fatal("invalid scaling factor: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-s/--scale",
+ 1, INT_MAX, &scale))
exit(1);
- }
break;
case 't':
benchmarking_option_set = true;
- nxacts = atoi(optarg);
- if (nxacts <= 0)
- {
- pg_log_fatal("invalid number of transactions: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-t/--transactions",
+ 1, INT_MAX, &nxacts))
exit(1);
- }
break;
case 'T':
benchmarking_option_set = true;
- duration = atoi(optarg);
- if (duration <= 0)
- {
- pg_log_fatal("invalid duration: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-T/--time",
+ 1, INT_MAX, &duration))
exit(1);
- }
break;
case 'U':
username = pg_strdup(optarg);
@@ -6019,12 +6011,9 @@ main(int argc, char **argv)
break;
case 'F':
initialization_option_set = true;
- fillfactor = atoi(optarg);
- if (fillfactor < 10 || fillfactor > 100)
- {
- pg_log_fatal("invalid fillfactor: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-F/--fillfactor",
+ 10, 100, &fillfactor))
exit(1);
- }
break;
case 'M':
benchmarking_option_set = true;
@@ -6039,38 +6028,61 @@ main(int argc, char **argv)
break;
case 'P':
benchmarking_option_set = true;
- progress = atoi(optarg);
- if (progress <= 0)
- {
- pg_log_fatal("invalid thread progress delay: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-P/--progress",
+ 1, INT_MAX, &progress))
exit(1);
- }
break;
case 'R':
{
- /* get a double from the beginning of option value */
- double throttle_value = atof(optarg);
+ double throttle_value;
- benchmarking_option_set = true;
+ if (option_parse_double(PG_LOG_FATAL, optarg, "-R/--rate",
+ &throttle_value))
+ exit(1);
- if (throttle_value <= 0.0)
+ /*
+ * This value will be used for yielding int64 number
+ * through some arithmetic. Quite arbitrary but people
+ * never want such small numbers for this parameter.
+ */
+ if (throttle_value <= 1e-5)
{
- pg_log_fatal("invalid rate limit: \"%s\"", optarg);
+ pg_log_fatal("-R/--rate must be greater than 1e-5: \"%s\"", optarg);
exit(1);
}
+
/* Invert rate limit into per-transaction delay in usec */
throttle_delay = 1000000.0 / throttle_value;
}
break;
case 'L':
{
- double limit_ms = atof(optarg);
+ double limit_ms;
+ if (option_parse_double(PG_LOG_FATAL, optarg,
+ "-L/--latency-limit", &limit_ms))
+ exit(1);
+
+ /* limit limit_ms so that latency_limit fits in int64 */
if (limit_ms <= 0.0)
{
- pg_log_fatal("invalid latency limit: \"%s\"", optarg);
+ pg_log_fatal("-L/--latency-limit must be greater than zero: \"%s\"",
+ optarg);
exit(1);
}
+ /*
+ * limit_ms * 1000 must fit int64. We could calculate the
+ * precise limit but also we don't need to accept such a
+ * large number here. Thus use a quite arbitrary seconds
+ * for the limit.
+ */
+ if (limit_ms > 3600)
+ {
+ pg_log_fatal("-L/--latency-limit must be less than 3600 seconds: \"%s\"",
+ optarg);
+ exit(1);
+ }
+
benchmarking_option_set = true;
latency_limit = (int64) (limit_ms * 1000);
}
@@ -6089,21 +6101,21 @@ main(int argc, char **argv)
break;
case 4: /* sampling-rate */
benchmarking_option_set = true;
- sample_rate = atof(optarg);
+ if (option_parse_double(PG_LOG_FATAL, optarg, "--samplig-rate",
+ &sample_rate))
+ exit(1);
if (sample_rate <= 0.0 || sample_rate > 1.0)
{
- pg_log_fatal("invalid sampling rate: \"%s\"", optarg);
+ pg_log_fatal("sampling rate must be greater than zero and less than or equal to 1.0: \"%s\"",
optarg);
exit(1);
}
break;
case 5: /* aggregate-interval */
benchmarking_option_set = true;
- agg_interval = atoi(optarg);
- if (agg_interval <= 0)
- {
- pg_log_fatal("invalid number of seconds for aggregation: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg,
+ "--aggregate-interval",
+ 1, INT_MAX, &agg_interval))
exit(1);
- }
break;
case 6: /* progress-timestamp */
progress_timestamp = true;
@@ -6135,12 +6147,9 @@ main(int argc, char **argv)
break;
case 11: /* partitions */
initialization_option_set = true;
- partitions = atoi(optarg);
- if (partitions < 0)
- {
- pg_log_fatal("invalid number of partitions: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "--partitions",
+ 0, INT_MAX, &partitions))
exit(1);
- }
break;
case 12: /* partition-method */
initialization_option_set = true;
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 49d4c0e3ce..312a797e78 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -11,6 +11,7 @@
#include <time.h>
#include <pwd.h>
#include <utime.h>
+#include <limits.h>
#ifndef WIN32
#include <sys/stat.h> /* for stat() */
#include <sys/time.h> /* for setitimer() */
@@ -34,6 +35,7 @@
#include "describe.h"
#include "fe_utils/cancel.h"
#include "fe_utils/print.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/string_utils.h"
#include "help.h"
#include "input.h"
@@ -1040,11 +1042,19 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
}
if (ln)
{
- lineno = atoi(ln);
- if (lineno < 1)
+ switch (option_parse_int(PG_LOG_OMIT, ln, "line number",
+ 1, INT_MAX, &lineno))
{
- pg_log_error("invalid line number: %s", ln);
- status = PSQL_CMD_ERROR;
+ case OPTPARSE_SUCCESS:
+ break;
+ case OPTPARSE_MALFORMED:
+ case OPTPARSE_TOOSMALL:
+ case OPTPARSE_TOOLARGE:
+ pg_log_error("line number must be an integer greater than zero: %s", ln);
+ break;
+ case OPTPARSE_OUTOFRANGE:
+ pg_log_error("line number out of range: %s", ln);
+ break;
}
}
if (status != PSQL_CMD_ERROR)
@@ -4284,7 +4294,16 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
else if (strcmp(param, "border") == 0)
{
if (value)
- popt->topt.border = atoi(value);
+ {
+ int v;
+
+ if (option_parse_int(PG_LOG_OMIT, value, "border", 0, 65535, &v))
+ {
+ pg_log_error("\\pset: border must be an integer in range 0..65535: \"%s\"", value);
+ return false;
+ }
+ popt->topt.border = v;
+ }
}
/* set expanded/vertical mode */
@@ -4440,7 +4459,22 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
else if (strcmp(param, "pager_min_lines") == 0)
{
if (value)
- popt->topt.pager_min_lines = atoi(value);
+ {
+ switch (option_parse_int(PG_LOG_OMIT, value, "page_min_lines",
+ 0, INT_MAX, &popt->topt.pager_min_lines))
+ {
+ case OPTPARSE_SUCCESS:
+ break;
+ case OPTPARSE_MALFORMED:
+ case OPTPARSE_TOOSMALL:
+ case OPTPARSE_TOOLARGE:
+ pg_log_error("\\pset: pager_min_lines must be a non-negative integer: \"%s\"", value);
+ return false;
+ case OPTPARSE_OUTOFRANGE:
+ pg_log_error("\\pset: pager_min_lines out of range: \"%s\"", value);
+ return false;
+ }
+ }
}
/* disable "(x rows)" footer */
@@ -4456,7 +4490,22 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
else if (strcmp(param, "columns") == 0)
{
if (value)
- popt->topt.columns = atoi(value);
+ {
+ switch (option_parse_int(PG_LOG_OMIT, value, "columns",
+ 0, INT_MAX, &popt->topt.columns))
+ {
+ case OPTPARSE_SUCCESS:
+ break;
+ case OPTPARSE_MALFORMED:
+ case OPTPARSE_TOOSMALL:
+ case OPTPARSE_TOOLARGE:
+ pg_log_error("\\pset: column must be a non-negative integer: \"%s\"", value);
+ return false;
+ case OPTPARSE_OUTOFRANGE:
+ pg_log_error("\\pset: column out of range: \"%s\"", value);
+ return false;
+ }
+ }
}
else
{
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index fc0681538a..f510a63157 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -9,6 +9,8 @@
*-------------------------------------------------------------------------
*/
+#include <limits.h>
+
#include "postgres_fe.h"
#include "catalog/pg_class_d.h"
@@ -151,12 +153,9 @@ main(int argc, char *argv[])
simple_string_list_append(&indexes, optarg);
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
- {
- pg_log_error("number of parallel jobs must be at least 1");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-j/--jobs",
+ 1, INT_MAX, &concurrentCons))
exit(1);
- }
break;
case 'v':
verbose = true;
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index a85919c5c1..21966ed7e2 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -10,6 +10,8 @@
*-------------------------------------------------------------------------
*/
+#include <limits.h>
+
#include "postgres_fe.h"
#include "catalog/pg_class_d.h"
@@ -17,6 +19,7 @@
#include "common.h"
#include "common/connect.h"
#include "common/logging.h"
+#include "common/string.h"
#include "fe_utils/cancel.h"
#include "fe_utils/option_utils.h"
#include "fe_utils/parallel_slot.h"
@@ -192,20 +195,14 @@ main(int argc, char *argv[])
vacopts.verbose = true;
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
- {
- pg_log_error("number of parallel jobs must be at least 1");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-j/--jobs",
+ 1, INT_MAX, &concurrentCons))
exit(1);
- }
break;
case 'P':
- vacopts.parallel_workers = atoi(optarg);
- if (vacopts.parallel_workers < 0)
- {
- pg_log_error("parallel workers for vacuum must be greater than or equal to zero");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-P/--parallel",
+ 0, INT_MAX, &vacopts.parallel_workers))
exit(1);
- }
break;
case 2:
maintenance_db = pg_strdup(optarg);
@@ -220,20 +217,14 @@ main(int argc, char *argv[])
vacopts.skip_locked = true;
break;
case 6:
- vacopts.min_xid_age = atoi(optarg);
- if (vacopts.min_xid_age <= 0)
- {
- pg_log_error("minimum transaction ID age must be at least 1");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "--min-xid-age",
+ 1, INT_MAX, &vacopts.min_xid_age))
exit(1);
- }
break;
case 7:
- vacopts.min_mxid_age = atoi(optarg);
- if (vacopts.min_mxid_age <= 0)
- {
- pg_log_error("minimum multixact ID age must be at least 1");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "--min-mxid-age",
+ 1, INT_MAX, &vacopts.min_mxid_age))
exit(1);
- }
break;
case 8:
vacopts.no_index_cleanup = true;
diff --git a/src/fe_utils/option_utils.c b/src/fe_utils/option_utils.c
index e19a495dba..4f191d321a 100644
--- a/src/fe_utils/option_utils.c
+++ b/src/fe_utils/option_utils.c
@@ -10,8 +10,12 @@
*-------------------------------------------------------------------------
*/
+#include <limits.h>
+
#include "postgres_fe.h"
+#include "common/string.h"
+#include "common/logging.h"
#include "fe_utils/option_utils.h"
/*
@@ -36,3 +40,151 @@ handle_help_version_opts(int argc, char *argv[],
}
}
}
+
+/*
+ * parser for int/long values in decimal for command line options
+ *
+ * Returns OPTPARSE_SUCCESS (0) when successfully parsed.
+ *
+ * Otherwise returns non-zero status numbers indicating error details after
+ * printing a generic error message with the given errorlevel, which may be
+ * omitted when the errorlevel is lower than the common/logging.c's
+ * setting. Giving PG_LOG_OMIT as loglevel disables generic errors.
+ *
+ * If result is not NULL, the result is stored there only when successfully
+ * parsed.
+ */
+optparse_result
+option_parse_int(int loglevel, const char *optarg, const char *optname,
+ int min_range, int max_range, int *result)
+{
+ char *endptr;
+ int parsed;
+
+ errno = 0;
+ parsed = strtoint(optarg, &endptr, 10);
+
+ if (*endptr)
+ {
+ pg_log_level(loglevel, "%s must be an integer: \"%s\"",
+ optname, optarg);
+ return OPTPARSE_MALFORMED;
+ }
+ else if (errno == ERANGE || parsed < min_range || parsed > max_range)
+ {
+ if (max_range == INT_MAX)
+ {
+ if (errno == ERANGE)
+ pg_log_level(loglevel, "%s out of range: %s",
+ optname, optarg);
+ else
+ pg_log_level(loglevel, "%s must be at least %d: %s",
+ optname, min_range, optarg);
+ }
+ else
+ pg_log_level(loglevel, "%s must be in range %d..%d: %s",
+ optname, min_range, max_range, optarg);
+
+ if (errno == ERANGE)
+ return OPTPARSE_OUTOFRANGE;
+ if (parsed < min_range)
+ return OPTPARSE_TOOSMALL;
+
+ Assert (parsed > max_range);
+ return OPTPARSE_TOOLARGE;
+ }
+
+ if (result)
+ *result = parsed;
+
+ return OPTPARSE_SUCCESS;
+}
+
+
+optparse_result
+option_parse_long(int loglevel, const char *optarg, const char *optname,
+ long min_range, long max_range, long *result)
+{
+ char *endptr;
+ long parsed;
+
+ errno = 0;
+ parsed = strtol(optarg, &endptr, 10);
+
+ if (*endptr)
+ {
+ pg_log_level(loglevel, "%s must be an integer: \"%s\"",
+ optname, optarg);
+ return OPTPARSE_MALFORMED;
+ }
+ else if (errno == ERANGE || parsed < min_range || parsed > max_range)
+ {
+ if (max_range == LONG_MAX)
+ {
+ if (errno == ERANGE)
+ pg_log_level(loglevel, "%s out of range: %s",
+ optname, optarg);
+ else
+ pg_log_level(loglevel, "%s must be at least %ld: %s",
+ optname, min_range, optarg);
+ }
+ else
+ pg_log_level(loglevel, "%s must be in range %ld..%ld: %s",
+ optname, min_range, max_range, optarg);
+
+ if (errno == ERANGE)
+ return OPTPARSE_OUTOFRANGE;
+ if (parsed < min_range)
+ return OPTPARSE_TOOSMALL;
+
+ Assert (parsed > max_range);
+ return OPTPARSE_TOOLARGE;
+ }
+
+ if (result)
+ *result = parsed;
+
+ return OPTPARSE_SUCCESS;
+}
+
+
+/*
+ * parser for double value in decimal for command line options
+ *
+ * Returns OPTPARSE_SUCCESS (0) when successfully parsed.
+ *
+ * Otherwise returns non-zero status numbers indicating error details after
+ * printing a fixed-form error message with the given errorlevel, which may be
+ * omitted when the errorlevel is lower than the common/logging.c's
+ * setting. Giving PG_LOG_OMIT as loglevel disables this error output.
+ *
+ * If result is not NULL, the result is stored there only when successfully
+ * parsed.
+ */
+optparse_result
+option_parse_double(int loglevel, const char *optarg, const char *optname,
+ double *result)
+{
+ char *endptr;
+ double parsed;
+
+ errno = 0;
+ parsed = strtod(optarg, &endptr);
+
+ if (*endptr)
+ {
+ pg_log_level(loglevel, "%s must be a numeric: \"%s\"", optname, optarg);
+ return OPTPARSE_MALFORMED;
+ }
+ else if (errno == ERANGE)
+ {
+ pg_log_level(loglevel, "%s out of range: %s", optname, optarg);
+
+ return OPTPARSE_OUTOFRANGE;
+ }
+
+ if (result)
+ *result = parsed;
+
+ return OPTPARSE_SUCCESS;
+}
diff --git a/src/include/common/logging.h b/src/include/common/logging.h
index a71cf84249..c2e9d1e46f 100644
--- a/src/include/common/logging.h
+++ b/src/include/common/logging.h
@@ -15,6 +15,11 @@
*/
enum pg_log_level
{
+ /*
+ * Don't show this message
+ */
+ PG_LOG_OMIT = -1,
+
/*
* Not initialized yet
*/
@@ -93,4 +98,8 @@ void pg_log_generic_v(enum pg_log_level level, const char *pg_restrict fmt, va_
if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) pg_log_generic(PG_LOG_DEBUG, __VA_ARGS__); \
} while(0)
+#define pg_log_level(loglevel, ...) do { \
+ if (unlikely(__pg_log_level <= loglevel)) pg_log_generic(loglevel, __VA_ARGS__); \
+ } while(0)
+
#endif /* COMMON_LOGGING_H */
diff --git a/src/include/fe_utils/option_utils.h b/src/include/fe_utils/option_utils.h
index d653cb94e3..bfba7a73ed 100644
--- a/src/include/fe_utils/option_utils.h
+++ b/src/include/fe_utils/option_utils.h
@@ -14,10 +14,33 @@
#include "postgres_fe.h"
+typedef enum optparse_result
+{
+ OPTPARSE_SUCCESS = 0,
+ OPTPARSE_MALFORMED,
+ OPTPARSE_TOOSMALL,
+ OPTPARSE_TOOLARGE,
+ OPTPARSE_OUTOFRANGE
+} optparse_result;
+
typedef void (*help_handler) (const char *progname);
extern void handle_help_version_opts(int argc, char *argv[],
const char *fixed_progname,
help_handler hlp);
+extern optparse_result option_parse_int(int loglevel,
+ const char *optarg, const char *optname,
+ int min_range, int max_range,
+ int *result);
+
+extern optparse_result option_parse_long(int loglevel,
+ const char *optarg,
+ const char *optname,
+ long min_range, long max_range,
+ long *result);
+
+extern optparse_result option_parse_double(int loglevel, const char *optarg,
+ const char *optname, double *result);
+
#endif /* OPTION_UTILS_H */
--
2.27.0
From 972567547f15e0f47512d08d97ce01aff4bc05a7 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 8 Jul 2021 17:10:19 +0900
Subject: [PATCH v3 2/3] Make complain for invalid numeirc values in
environemnt variables
Postgresql commands that read environment variables for integer values
are tolerant of trailing garbages like '5432xyz'. Ignore such values
and make warning on it.
---
src/bin/pg_ctl/pg_ctl.c | 6 ++++--
src/bin/pg_upgrade/option.c | 26 ++++++++++++++++++++++++--
src/bin/psql/startup.c | 11 ++++++++++-
3 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index f7ccdcdc96..b3c3e66bda 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2320,8 +2320,10 @@ main(int argc, char **argv)
#endif
env_wait = getenv("PGCTLTIMEOUT");
- if (env_wait != NULL)
- wait_seconds = atoi(env_wait);
+ if (env_wait != NULL &&
+ option_parse_int(PG_LOG_OMIT, env_wait, "PGCTLTIMEOUT",
+ 0, 65535, &wait_seconds))
+ write_stderr(_("%s: ignored invalid setting of environment variable PGCTLTIMEOUT: %s\n"), progname,
env_wait);
/*
* 'Action' can be before or after args so loop over both. Some
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index a8cd23f7fa..e506c3e2af 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -74,8 +74,30 @@ parseCommandLine(int argc, char *argv[])
os_info.progname = get_progname(argv[0]);
/* Process libpq env. variables; load values here for usage() output */
- old_cluster.port = getenv("PGPORTOLD") ? atoi(getenv("PGPORTOLD")) : DEF_PGUPORT;
- new_cluster.port = getenv("PGPORTNEW") ? atoi(getenv("PGPORTNEW")) : DEF_PGUPORT;
+ old_cluster.port = new_cluster.port = DEF_PGUPORT;
+ if (getenv("PGPORTOLD"))
+ {
+ int p;
+
+ if (option_parse_int(PG_LOG_OMIT, getenv("PGPORTOLD"), "PGPORTOLD",
+ 0, 65535, &p))
+ pg_log(PG_WARNING,
+ "%s: ignored invalid setting of environment variable PGPORTOLD: %s\n",
+ os_info.progname, getenv("PGPORTOLD"));
+ old_cluster.port = p;
+ }
+
+ if (getenv("PGPORTNEW"))
+ {
+ int p;
+
+ if (option_parse_int(PG_LOG_OMIT, getenv("PGPORTNEW"), "PGPORTNEW",
+ 0, 65535, &p))
+ pg_log(PG_WARNING,
+ "%s: ignored invalid setting of environment variable PGPORTNEW: %s\n",
+ os_info.progname, getenv("PGPORTNEW"));
+ new_cluster.port = p;
+ }
os_user_effective_id = get_user_info(&os_info.user);
/* we override just the database user name; we got the OS id above */
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 2931530f33..819d5493c9 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -13,12 +13,14 @@
#include <io.h>
#include <win32.h>
#endif /* WIN32 */
+#include <limits.h>
#include "command.h"
#include "common.h"
#include "common/logging.h"
#include "common/string.h"
#include "describe.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/print.h"
#include "getopt_long.h"
#include "help.h"
@@ -181,7 +183,14 @@ main(int argc, char *argv[])
refresh_utf8format(&(pset.popt.topt));
/* We must get COLUMNS here before readline() sets it */
- pset.popt.topt.env_columns = getenv("COLUMNS") ? atoi(getenv("COLUMNS")) : 0;
+ pset.popt.topt.env_columns = 0;
+ if (getenv("COLUMNS"))
+ {
+ if (option_parse_int(PG_LOG_OMIT, getenv("COLUMNS"), "COLUMNS",
+ 0, INT_MAX, &pset.popt.topt.env_columns))
+ pg_log_warning("ignored invalid setting of environemt variable COLUMNS: %s",
+ getenv("COLUMNS"));
+ }
pset.notty = (!isatty(fileno(stdin)) || !isatty(fileno(stdout)));
--
2.27.0
From ae7276bd8df4e9d9f1d37156cfa29f1130698403 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Tue, 13 Jul 2021 19:35:12 +0900
Subject: [PATCH v3 3/3] Doc change for pg_ctl
The previous patch change the bahavior of pg_ctl about
--timeout. Change the doc following the behavior change.
---
doc/src/sgml/ref/pg_ctl-ref.sgml | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/ref/pg_ctl-ref.sgml b/doc/src/sgml/ref/pg_ctl-ref.sgml
index 3946fa52ea..90e0fabc63 100644
--- a/doc/src/sgml/ref/pg_ctl-ref.sgml
+++ b/doc/src/sgml/ref/pg_ctl-ref.sgml
@@ -389,9 +389,11 @@ PostgreSQL documentation
<listitem>
<para>
Specifies the maximum number of seconds to wait when waiting for an
- operation to complete (see option <option>-w</option>). Defaults to
- the value of the <envar>PGCTLTIMEOUT</envar> environment variable or, if
- not set, to 60 seconds.
+ operation to complete (see option <option>-w</option>). If it is set
+ to zero, <command>pg_ctl</command> behaves the same way
+ with <option>-W</option>. Defaults to the value of
+ the <envar>PGCTLTIMEOUT</envar> environment variable or, if not set,
+ to 60 seconds.
</para>
</listitem>
</varlistentry>
--
2.27.0
pgsql-hackers by date: