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 | 20210709.165028.1509934550749179556.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 |
Thank you for the comments.
At Fri, 9 Jul 2021 10:29:07 +0900, Michael Paquier <michael@paquier.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
From 0818de16f9febea3d90ae0404b4f5b3643f6cbe0 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 v2 1/2] 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 | 6 ++-
src/bin/pg_basebackup/pg_basebackup.c | 13 +++--
src/bin/pg_basebackup/pg_receivewal.c | 18 +++++--
src/bin/pg_basebackup/pg_recvlogical.c | 17 +++++--
src/bin/pg_checksums/pg_checksums.c | 7 ++-
src/bin/pg_ctl/pg_ctl.c | 18 ++++++-
src/bin/pg_dump/pg_dump.c | 39 ++++++++-------
src/bin/pg_dump/pg_restore.c | 17 ++++---
src/bin/pg_upgrade/option.c | 21 ++++++--
src/bin/pgbench/pgbench.c | 66 ++++++++++++++++----------
src/bin/psql/command.c | 52 ++++++++++++++++++--
src/bin/scripts/reindexdb.c | 10 ++--
src/bin/scripts/vacuumdb.c | 23 +++++----
13 files changed, 219 insertions(+), 88 deletions(-)
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 4bde16fb4b..71a82f9b75 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -17,6 +17,7 @@
#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,8 +327,9 @@ main(int argc, char *argv[])
append_btree_pattern(&opts.exclude, optarg, encoding);
break;
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);
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8bb0acf498..29be95b96a 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -2287,6 +2287,8 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
long_options, &option_index)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'C':
@@ -2371,8 +2373,10 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ errno = 0;
+ compresslevel = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ compresslevel < 0 || compresslevel > 9)
{
pg_log_error("invalid compression level \"%s\"", optarg);
exit(1);
@@ -2409,8 +2413,9 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ errno = 0;
+ standby_message_timeout = strtoint(optarg, &endptr, 10) * 1000;
+ if (*endptr || errno == ERANGE || standby_message_timeout < 0)
{
pg_log_error("invalid status interval \"%s\"", optarg);
exit(1);
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index c1334fad35..7fef925b99 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -22,6 +22,7 @@
#include "access/xlog_internal.h"
#include "common/file_perm.h"
#include "common/logging.h"
+#include "common/string.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "receivelog.h"
@@ -520,6 +521,8 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "D:d:E:h:p:U:s:S:nwWvZ:",
long_options, &option_index)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'D':
@@ -532,7 +535,9 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ errno = 0;
+ if (strtoint(optarg, &endptr, 10) <= 0 ||
+ *endptr || errno == ERANGE)
{
pg_log_error("invalid port number \"%s\"", optarg);
exit(1);
@@ -549,8 +554,9 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ errno = 0;
+ standby_message_timeout = strtoint(optarg, &endptr, 10) * 1000;
+ if (*endptr || errno == ERANGE || standby_message_timeout < 0)
{
pg_log_error("invalid status interval \"%s\"", optarg);
exit(1);
@@ -574,8 +580,10 @@ main(int argc, char **argv)
verbose++;
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ errno = 0;
+ compresslevel = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ compresslevel < 0 || compresslevel > 9)
{
pg_log_error("invalid compression level \"%s\"", optarg);
exit(1);
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 76bd153fac..7be932d025 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -23,6 +23,7 @@
#include "common/fe_memutils.h"
#include "common/file_perm.h"
#include "common/logging.h"
+#include "common/string.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "libpq/pqsignal.h"
@@ -732,6 +733,8 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "E:f:F:nvtd:h:p:U:wWI:o:P:s:S:",
long_options, &option_index)) != -1)
{
+ char *endptr;
+
switch (c)
{
/* general options */
@@ -739,8 +742,9 @@ main(int argc, char **argv)
outfile = pg_strdup(optarg);
break;
case 'F':
- fsync_interval = atoi(optarg) * 1000;
- if (fsync_interval < 0)
+ errno = 0;
+ fsync_interval = strtoint(optarg, &endptr, 10) * 1000;
+ if (*endptr || errno == ERANGE || fsync_interval < 0)
{
pg_log_error("invalid fsync interval \"%s\"", optarg);
exit(1);
@@ -763,7 +767,9 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ errno = 0;
+ if (strtoint(optarg, &endptr, 10) <= 0 ||
+ *endptr || errno == ERANGE)
{
pg_log_error("invalid port number \"%s\"", optarg);
exit(1);
@@ -820,8 +826,9 @@ main(int argc, char **argv)
plugin = pg_strdup(optarg);
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ errno = 0;
+ standby_message_timeout = strtoint(optarg, &endptr, 10) * 1000;
+ if (*endptr || errno == ERANGE || standby_message_timeout < 0)
{
pg_log_error("invalid status interval \"%s\"", optarg);
exit(1);
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 3c326906e2..1c4e5b9d85 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -24,6 +24,7 @@
#include "common/file_perm.h"
#include "common/file_utils.h"
#include "common/logging.h"
+#include "common/string.h"
#include "getopt_long.h"
#include "pg_getopt.h"
#include "storage/bufpage.h"
@@ -506,6 +507,8 @@ main(int argc, char *argv[])
while ((c = getopt_long(argc, argv, "cD:deNPf:v", long_options, &option_index)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'c':
@@ -518,7 +521,9 @@ main(int argc, char *argv[])
mode = PG_MODE_ENABLE;
break;
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);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 7985da0a94..d6a39182cf 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2331,6 +2331,8 @@ main(int argc, char **argv)
/* process command-line options */
while (optind < argc)
{
+ char *endptr;
+
while ((c = getopt_long(argc, argv, "cD:e:l:m:N:o:p:P:sS:t:U:wW",
long_options, &option_index)) != -1)
{
@@ -2396,7 +2398,13 @@ main(int argc, char **argv)
#endif
break;
case 't':
- wait_seconds = atoi(optarg);
+ errno = 0;
+ wait_seconds = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || wait_seconds < 1)
+ {
+ pg_log_error("invalid timeout value \"%s\", use --no-wait to finish without waiting",
optarg);
+ exit(1);
+ }
wait_seconds_arg = true;
break;
case 'U':
@@ -2459,7 +2467,13 @@ main(int argc, char **argv)
}
ctl_command = KILL_COMMAND;
set_sig(argv[++optind]);
- 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);
+ }
}
#ifdef WIN32
else if (strcmp(argv[optind], "register") == 0)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 321152151d..793f4b3509 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -54,6 +54,7 @@
#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/string_utils.h"
#include "getopt_long.h"
@@ -486,7 +487,19 @@ main(int argc, char **argv)
break;
case 'j': /* number of dump jobs */
- numWorkers = atoi(optarg);
+ errno = 0;
+ numWorkers = strtoint(optarg, &endptr, 10);
+ /*
+ * 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 (*endptr || errno == ERANGE || numWorkers <= 0
+#ifdef WIN32
+ || numWorkers > MAXIMUM_WAIT_OBJECTS
+#endif
+ )
+ fatal("invalid number of parallel jobs %s", optarg);
break;
case 'n': /* include schema(s) */
@@ -549,8 +562,10 @@ main(int argc, char **argv)
break;
case 'Z': /* Compression Level */
- compressLevel = atoi(optarg);
- if (compressLevel < 0 || compressLevel > 9)
+ errno = 0;
+ compressLevel = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ compressLevel < 0 || compressLevel > 9)
{
pg_log_error("compression level must be in range 0..9");
exit_nicely(1);
@@ -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);
@@ -719,18 +736,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..285a09aaac 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -39,6 +39,7 @@
*-------------------------------------------------------------------------
*/
#include "postgres_fe.h"
+#include "common/string.h"
#include <ctype.h>
#ifdef HAVE_TERMIOS_H
@@ -151,6 +152,8 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:N:Op:P:RsS:t:T:U:vwWx1",
cmdopts, NULL)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'a': /* Dump data only */
@@ -181,7 +184,13 @@ main(int argc, char **argv)
break;
case 'j': /* number of restore jobs */
- numWorkers = atoi(optarg);
+ errno = 0;
+ numWorkers = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || numWorkers <= 0)
+ {
+ pg_log_error("invalid number of parallel jobs");
+ exit(1);
+ }
break;
case 'l': /* Dump the TOC summary */
@@ -344,12 +353,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)
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 64bbda5650..f96f0d1e2a 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -104,6 +104,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)
{
+ char *endptr;
+
switch (option)
{
case 'b':
@@ -127,7 +129,12 @@ parseCommandLine(int argc, char *argv[])
break;
case 'j':
- user_opts.jobs = atoi(optarg);
+ errno = 0;
+ user_opts.jobs = strtoint(optarg, &endptr, 10);
+ /**/
+ if (*endptr || errno == ERANGE || user_opts.jobs < 1)
+ pg_fatal("invalid number of jobs %s\n", optarg);
+
break;
case 'k':
@@ -166,13 +173,17 @@ 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");
+ errno = 0;
+ if ((old_cluster.port = strtoint(optarg, &endptr, 10)) <= 0 ||
+ *endptr || errno == ERANGE)
+ pg_fatal("invalid old port number %s\n", optarg);
break;
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;
case 'r':
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4aeccd93af..4020347585 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -5838,6 +5838,7 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) !=
-1)
{
char *script;
+ char *endptr;
switch (c)
{
@@ -5869,8 +5870,9 @@ main(int argc, char **argv)
break;
case 'c':
benchmarking_option_set = true;
- nclients = atoi(optarg);
- if (nclients <= 0)
+ errno = 0;
+ nclients = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || nclients <= 0)
{
pg_log_fatal("invalid number of clients: \"%s\"", optarg);
exit(1);
@@ -5896,8 +5898,9 @@ main(int argc, char **argv)
break;
case 'j': /* jobs */
benchmarking_option_set = true;
- nthreads = atoi(optarg);
- if (nthreads <= 0)
+ errno = 0;
+ nthreads = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || nthreads <= 0)
{
pg_log_fatal("invalid number of threads: \"%s\"", optarg);
exit(1);
@@ -5920,8 +5923,9 @@ main(int argc, char **argv)
break;
case 's':
scale_given = true;
- scale = atoi(optarg);
- if (scale <= 0)
+ errno = 0;
+ scale = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || scale <= 0)
{
pg_log_fatal("invalid scaling factor: \"%s\"", optarg);
exit(1);
@@ -5929,8 +5933,9 @@ main(int argc, char **argv)
break;
case 't':
benchmarking_option_set = true;
- nxacts = atoi(optarg);
- if (nxacts <= 0)
+ errno = 0;
+ nxacts = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || nxacts <= 0)
{
pg_log_fatal("invalid number of transactions: \"%s\"", optarg);
exit(1);
@@ -5938,8 +5943,9 @@ main(int argc, char **argv)
break;
case 'T':
benchmarking_option_set = true;
- duration = atoi(optarg);
- if (duration <= 0)
+ errno = 0;
+ duration = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || duration <= 0)
{
pg_log_fatal("invalid duration: \"%s\"", optarg);
exit(1);
@@ -6001,8 +6007,10 @@ main(int argc, char **argv)
break;
case 'F':
initialization_option_set = true;
- fillfactor = atoi(optarg);
- if (fillfactor < 10 || fillfactor > 100)
+ errno = 0;
+ fillfactor = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ fillfactor < 10 || fillfactor > 100)
{
pg_log_fatal("invalid fillfactor: \"%s\"", optarg);
exit(1);
@@ -6021,8 +6029,9 @@ main(int argc, char **argv)
break;
case 'P':
benchmarking_option_set = true;
- progress = atoi(optarg);
- if (progress <= 0)
+ errno = 0;
+ progress = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || progress <= 0)
{
pg_log_fatal("invalid thread progress delay: \"%s\"", optarg);
exit(1);
@@ -6031,11 +6040,13 @@ main(int argc, char **argv)
case 'R':
{
/* get a double from the beginning of option value */
- double throttle_value = atof(optarg);
+ double throttle_value;
+ errno = 0;
+ throttle_value = strtod(optarg, &endptr);
benchmarking_option_set = true;
- if (throttle_value <= 0.0)
+ if (*endptr || errno == ERANGE || throttle_value <= 0.0)
{
pg_log_fatal("invalid rate limit: \"%s\"", optarg);
exit(1);
@@ -6046,9 +6057,12 @@ main(int argc, char **argv)
break;
case 'L':
{
- double limit_ms = atof(optarg);
+ double limit_ms;
- if (limit_ms <= 0.0)
+ errno = 0;
+ limit_ms = strtod(optarg, &endptr);
+
+ if (*endptr || errno == ERANGE || limit_ms <= 0.0)
{
pg_log_fatal("invalid latency limit: \"%s\"", optarg);
exit(1);
@@ -6071,8 +6085,10 @@ main(int argc, char **argv)
break;
case 4: /* sampling-rate */
benchmarking_option_set = true;
- sample_rate = atof(optarg);
- if (sample_rate <= 0.0 || sample_rate > 1.0)
+ errno = 0;
+ sample_rate = strtod(optarg, &endptr);
+ if (*endptr || errno == ERANGE ||
+ sample_rate <= 0.0 || sample_rate > 1.0)
{
pg_log_fatal("invalid sampling rate: \"%s\"", optarg);
exit(1);
@@ -6080,8 +6096,9 @@ main(int argc, char **argv)
break;
case 5: /* aggregate-interval */
benchmarking_option_set = true;
- agg_interval = atoi(optarg);
- if (agg_interval <= 0)
+ errno = 0;
+ agg_interval = strtod(optarg, &endptr);
+ if (*endptr || errno == ERANGE || agg_interval <= 0)
{
pg_log_fatal("invalid number of seconds for aggregation: \"%s\"", optarg);
exit(1);
@@ -6117,8 +6134,9 @@ main(int argc, char **argv)
break;
case 11: /* partitions */
initialization_option_set = true;
- partitions = atoi(optarg);
- if (partitions < 0)
+ errno = 0;
+ partitions = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || partitions < 0)
{
pg_log_fatal("invalid number of partitions: \"%s\"", optarg);
exit(1);
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 543401c6d6..aaed986ae1 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1039,8 +1039,11 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
}
if (ln)
{
- lineno = atoi(ln);
- if (lineno < 1)
+ char *endptr;
+
+ errno = 0;
+ lineno = strtoint(ln, &endptr, 10);
+ if (*endptr || errno == ERANGE || lineno < 1)
{
pg_log_error("invalid line number: %s", ln);
status = PSQL_CMD_ERROR;
@@ -4283,7 +4286,21 @@ 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);
+ {
+ char *endptr;
+ int new_value;
+
+ errno = 0;
+ new_value = strtoint(value, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ new_value < 0 || new_value > 65535)
+ {
+ pg_log_error("\\pset: border is invalid or out of range");
+ return false;
+ }
+
+ popt->topt.border = new_value;
+ }
}
/* set expanded/vertical mode */
@@ -4439,7 +4456,20 @@ 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);
+ {
+ char *endptr;
+ int new_value;
+
+ errno = 0;
+ new_value = strtoint(value, &endptr, 10);
+ if (*endptr || errno == ERANGE || new_value < 0)
+ {
+ pg_log_error("\\pset: pager_min_lines is invalid or out of range");
+ return false;
+ }
+
+ popt->topt.pager_min_lines = new_value;
+ }
}
/* disable "(x rows)" footer */
@@ -4455,7 +4485,19 @@ 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);
+ {
+ char *endptr;
+ int new_value;
+
+ errno = 0;
+ new_value = strtoint(value, &endptr, 10);
+ if (*endptr || errno == ERANGE || new_value < 0)
+ {
+ pg_log_error("\\pset: column is invalid or out of range");
+ return false;
+ }
+ popt->topt.columns = new_value;
+ }
}
else
{
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index fc0681538a..baa68d58d8 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -15,6 +15,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"
@@ -109,6 +110,8 @@ main(int argc, char *argv[])
/* process command-line options */
while ((c = getopt_long(argc, argv, "h:p:U:wWeqS:d:ast:i:j:v", long_options, &optindex)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'h':
@@ -151,10 +154,11 @@ main(int argc, char *argv[])
simple_string_list_append(&indexes, optarg);
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
+ errno = 0;
+ concurrentCons = strtoint(optarg, &endptr, 10);
+ 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);
}
break;
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 61974baa78..93b563998a 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -17,6 +17,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"
@@ -141,6 +142,8 @@ main(int argc, char *argv[])
while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:zZFat:fvj:P:", long_options, &optindex)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'h':
@@ -192,16 +195,18 @@ main(int argc, char *argv[])
vacopts.verbose = true;
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
+ errno = 0;
+ 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;
case 'P':
- vacopts.parallel_workers = atoi(optarg);
- if (vacopts.parallel_workers < 0)
+ errno = 0;
+ vacopts.parallel_workers = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || vacopts.parallel_workers < 0)
{
pg_log_error("parallel workers for vacuum must be greater than or equal to zero");
exit(1);
@@ -220,16 +225,18 @@ main(int argc, char *argv[])
vacopts.skip_locked = true;
break;
case 6:
- vacopts.min_xid_age = atoi(optarg);
- if (vacopts.min_xid_age <= 0)
+ errno = 0;
+ vacopts.min_xid_age = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || vacopts.min_xid_age <= 0)
{
pg_log_error("minimum transaction ID age must be at least 1");
exit(1);
}
break;
case 7:
- vacopts.min_mxid_age = atoi(optarg);
- if (vacopts.min_mxid_age <= 0)
+ errno = 0;
+ vacopts.min_mxid_age = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || vacopts.min_mxid_age <= 0)
{
pg_log_error("minimum multixact ID age must be at least 1");
exit(1);
--
2.27.0
From 039490f1bb8a866be62276fdfb50c322d7686351 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 v2 2/2] 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 | 12 +++++++++++-
src/bin/pg_upgrade/option.c | 32 ++++++++++++++++++++++++++++++--
src/bin/psql/startup.c | 17 ++++++++++++++++-
3 files changed, 57 insertions(+), 4 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index d6a39182cf..3025eb31bd 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2318,7 +2318,17 @@ main(int argc, char **argv)
env_wait = getenv("PGCTLTIMEOUT");
if (env_wait != NULL)
- wait_seconds = atoi(env_wait);
+ {
+ char *endptr;
+ int tmp_wait;
+
+ errno = 0;
+ tmp_wait = strtoint(env_wait, &endptr, 10);
+ if (*endptr || errno == ERANGE || tmp_wait < 1)
+ write_stderr(_("%s: ignored invalid setting of environment variable PGCTLTIMEOUT: %s\n"), progname,
env_wait);
+ else
+ wait_seconds = tmp_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 f96f0d1e2a..869f119456 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -71,8 +71,36 @@ 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"))
+ {
+ char *endptr;
+ int port;
+
+ errno = 0;
+ port = strtoint(getenv("PGPORTOLD"), &endptr, 10);
+ if (*endptr || errno == ERANGE || port < 0)
+ pg_log(PG_WARNING,
+ "%s: ignored invalid setting of environment variable PGPORTOLD: %s\n",
+ os_info.progname, getenv("PGPORTOLD"));
+ else
+ old_cluster.port = port;
+ }
+
+ if (getenv("PGPORTNEW"))
+ {
+ char *endptr;
+ int port;
+
+ errno = 0;
+ port = strtoint(getenv("PGPORTNEW"), &endptr, 10);
+ if (*endptr || errno == ERANGE || port < 0)
+ pg_log(PG_WARNING,
+ "%s: ignored invalid setting of environment variable PGPORTNEW: %s\n",
+ os_info.progname, getenv("PGPORTNEW"));
+ else
+ new_cluster.port = port;
+ }
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 110906a4e9..4378c5daa3 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -174,7 +174,22 @@ 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"))
+ {
+ char *endptr;
+ int cols;
+
+ errno = 0;
+ cols = strtoint(getenv("COLUMNS"), &endptr, 10);
+ if (*endptr || errno == ERANGE || cols < 0)
+ {
+ pg_log_warning("ignored invalid setting of environemt variable COLUMNS: %s",
+ getenv("COLUMNS"));
+ }
+
+ pset.popt.topt.env_columns = cols;
+ }
pset.notty = (!isatty(fileno(stdin)) || !isatty(fileno(stdout)));
--
2.27.0
pgsql-hackers by date: