Re: Incorrect usage of strtol, atoi for non-numeric junk inputs - Mailing list pgsql-hackers

At Wed, 7 Jul 2021 17:40:13 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in 
> On Fri, Jun 4, 2021 at 10:23 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > On 2021-Jun-04, Bharath Rupireddy wrote:
> >
> > > On Fri, Jun 4, 2021 at 8:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > > > I would suggest that the best way forward in this area is to rebase both
> > > > there patches on current master.
> > >
> > > Thanks. I will read both the threads [1], [2] and try to rebase the
> > > patches. If at all I get to rebase them, do you prefer the patches to
> > > be in this thread or in a new thread?
> >
> > Thanks, that would be helpful.  This thread is a good place.
> 
> I'm unable to spend time on this work as promised. I'd be happy if
> someone could take it forward, although it's not critical work(IMO)
> that needs immediate focus. I will try to spend time maybe later this
> year.

Looked through the three threads.

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

So I agree to the Bharath's point.

So the attached is a standalone patch that:

- doesn't contain [1], since that functions are not needed for this
  purpose.

- basically does the same thing with [2], but uses
  strtoint/strtol/strtod instead of pg_strtoint16/32.

- doesn't try to make error messages verbose. That results in a
  somewhat strange message like this but I'm not sure we should be so
  strict at that point.

  > reindexdb: error: number of parallel jobs must be at least 1: hoge

- is extended to cover all usages of atoi/l/f in command line
  processing, which are not fully covered by [2]. (Maybe)

- is extended to cover psql's meta command parameters.

- is extended to cover integral environment variables. (PGPORTOLD/NEW
  of pg_upgrade and COLUMNS of psql). The commands emit a warning for
  invalid values, but I'm not sure it's worthwhile. (The second attached.)

  > psql: warning: ignored invalid setting of environemt variable COLUMNS: 3x

- doesn't cover pgbench's meta command parameters (for speed).


[1] - https://www.postgresql.org/message-id/flat/alpine.DEB.2.21.1904201223040.29102@lancre
[2] - https://www.postgresql.org/message-id/20191028012000.GA59064@begriffs.com

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 54ed83143012473727faff9f9e64dee613cedcfe 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 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..78882dec48 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 < 0)
+                    {
+                        pg_log_error("invalid timeout \"%s\"", 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..357ba04f4f 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)
+                    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 17d541ccc9cd55ff7b547d02b72099e9a26268dd 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 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_upgrade/option.c | 32 ++++++++++++++++++++++++++++++--
 src/bin/psql/startup.c      | 17 ++++++++++++++++-
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 357ba04f4f..95a979004d 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