On Friday, April 14, 2017 8:59:03 AM CEST Robert Haas wrote: > On Fri, Apr 14, 2017 at 2:32 AM, Pierre Ducroquet <p.p...@pinaraf.info> wrote: > > On Friday, April 14, 2017 8:44:37 AM CEST Michael Paquier wrote: > >> On Fri, Apr 14, 2017 at 6:32 AM, Pierre Ducroquet <p.p...@pinaraf.info> > > > > wrote: > >> > Yesterday while doing a few pg_basebackup, I realized that the integer > >> > parameters were not properly checked against invalid input. > >> > It is not a critical issue, but this could be misleading for an user > >> > who > >> > writes -z max or -s 0.5… > >> > I've attached the patch to this mail. Should I add it to the next > >> > commit > >> > fest or is it not needed for such small patches ? > >> > >> A call to atoi is actually equivalent to strtol with the rounding: > >> (int)strtol(str, (char **)NULL, 10); > >> So I don't think this is worth caring. > > > > The problem with atoi is that it simply ignores any invalid input and > > returns 0 instead. > > That's why I did this patch, because I did a typo when calling > > pg_basebackup and was not warned for an invalid input. > > I agree. I think it would be worth going through and cleaning up > every instance of this in the source tree.
Hi Following your advice, I went through the source tree and cleaned up most instances of that pattern. I have attached the corresponding patch to this mail. If you think this patch is indeed interesting, what would be the next way to have it reviewed ? Thanks Pierre
From da448337bd050518323b45fd5d2929d04b552802 Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet <pierre.ducroq...@people-doc.com>
Date: Tue, 18 Apr 2017 09:40:40 +0200
Subject: [PATCH] Port most calls of atoi(optarg) to strcol
atoi does not allow any kind of data check. If the input data is invalid,
the result will be silently truncated to the valid part of input, or just
0 if no digit was found at the beginning of input.
---
src/bin/pg_basebackup/pg_basebackup.c | 11 ++++++-----
src/bin/pg_basebackup/pg_receivewal.c | 11 ++++++-----
src/bin/pg_basebackup/pg_recvlogical.c | 11 ++++++-----
src/bin/pg_ctl/pg_ctl.c | 8 +++++++-
src/bin/pg_dump/pg_dump.c | 12 +++++++++---
src/bin/pg_dump/pg_restore.c | 8 +++++++-
src/bin/pg_upgrade/option.c | 5 +++--
src/bin/pgbench/pgbench.c | 34 ++++++++++++++++++----------------
8 files changed, 62 insertions(+), 38 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 40ec0e17dc..34884fce4f 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -87,7 +87,7 @@ static IncludeWal includewal = STREAM_WAL;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
static bool do_sync = true;
-static int standby_message_timeout = 10 * 1000; /* 10 sec = default */
+static long int standby_message_timeout = 10 * 1000; /* 10 sec = default */
static pg_time_t last_progress_report = 0;
static int32 maxrate = 0; /* no limit by default */
static char *replication_slot = NULL;
@@ -2063,6 +2063,7 @@ BaseBackup(void)
int
main(int argc, char **argv)
{
+ char *strtol_endptr = NULL;
static struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"version", no_argument, NULL, 'V'},
@@ -2203,8 +2204,8 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ compresslevel = strtol(optarg, &strtol_endptr, 10);
+ if (compresslevel < 0 || compresslevel > 9 || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
@@ -2242,8 +2243,8 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 1a9fe81be1..f470406678 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -479,6 +479,7 @@ main(int argc, char **argv)
int c;
int option_index;
char *db_name;
+ char *strtol_endptr = NULL;
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
@@ -513,7 +514,7 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ if ((strtol(optarg, &strtol_endptr, 10) <= 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid port number \"%s\"\n"),
progname, optarg);
@@ -531,8 +532,8 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
@@ -549,8 +550,8 @@ main(int argc, char **argv)
verbose++;
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ compresslevel = strtol(optarg, &strtol_endptr, 10);
+ if (compresslevel < 0 || compresslevel > 9 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 6b081bd737..cd69a6fba6 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -706,6 +706,7 @@ main(int argc, char **argv)
uint32 hi,
lo;
char *db_name;
+ char *strtol_endptr;
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
@@ -735,8 +736,8 @@ main(int argc, char **argv)
outfile = pg_strdup(optarg);
break;
case 'F':
- fsync_interval = atoi(optarg) * 1000;
- if (fsync_interval < 0)
+ fsync_interval = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if (fsync_interval < 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid fsync interval \"%s\"\n"),
progname, optarg);
@@ -757,7 +758,7 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ if (strtol(optarg, &strtol_endptr, 10) <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid port number \"%s\"\n"),
progname, optarg);
@@ -819,8 +820,8 @@ main(int argc, char **argv)
plugin = pg_strdup(optarg);
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if (standby_message_timeout < 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index c63819b88b..65191a5319 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2177,6 +2177,7 @@ main(int argc, char **argv)
};
char *env_wait;
+ char *strtol_endptr;
int option_index;
int c;
pgpid_t killproc = 0;
@@ -2307,7 +2308,12 @@ main(int argc, char **argv)
#endif
break;
case 't':
- wait_seconds = atoi(optarg);
+ wait_seconds = strtol(optarg, &strtol_endptr, 10);
+ if (strtol_endptr != optarg + strlen(optarg)) {
+ write_stderr(_("%s: invalid value '%s' for -t\n"),
+ progname, optarg);
+ exit(1);
+ }
wait_seconds_arg = true;
break;
case 'U':
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index e9b5c8a448..97a4ac5f44 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -296,6 +296,7 @@ main(int argc, char **argv)
int plainText = 0;
ArchiveFormat archiveFormat = archUnknown;
ArchiveMode archiveMode;
+ char *strtol_endptr = NULL;
static DumpOptions dopt;
@@ -438,7 +439,12 @@ main(int argc, char **argv)
break;
case 'j': /* number of dump jobs */
- numWorkers = atoi(optarg);
+ numWorkers = strtol(optarg, &strtol_endptr, 10);
+ if (strtol_endptr != optarg + strlen(optarg))
+ {
+ write_msg(NULL, "invalid number of jobs\n");
+ exit_nicely(1);
+ }
break;
case 'n': /* include schema(s) */
@@ -504,8 +510,8 @@ main(int argc, char **argv)
break;
case 'Z': /* Compression Level */
- compressLevel = atoi(optarg);
- if (compressLevel < 0 || compressLevel > 9)
+ compressLevel = strtol(optarg, &strtol_endptr, 10);
+ if (compressLevel < 0 || compressLevel > 9 || strtol_endptr != optarg + strlen(optarg))
{
write_msg(NULL, "compression level must be in range 0..9\n");
exit_nicely(1);
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index ddd79429b4..dac5f7b2de 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -73,6 +73,7 @@ main(int argc, char **argv)
static int use_setsessauth = 0;
static int no_security_labels = 0;
static int strict_names = 0;
+ char *strtol_endptr = NULL;
struct option cmdopts[] = {
{"clean", 0, NULL, 'c'},
@@ -177,7 +178,12 @@ main(int argc, char **argv)
break;
case 'j': /* number of restore jobs */
- numWorkers = atoi(optarg);
+ numWorkers = strtol(optarg, &strtol_endptr, 10);
+ if (strtol_endptr != optarg + strlen(optarg))
+ {
+ write_msg(NULL, "invalid number of jobs\n");
+ exit_nicely(1);
+ }
break;
case 'l': /* Dump the TOC summary */
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 5007ce53cf..b4acbdc727 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -60,6 +60,7 @@ parseCommandLine(int argc, char *argv[])
int os_user_effective_id;
FILE *fp;
char **filename;
+ char *strtol_endptr;
time_t run_time = time(NULL);
user_opts.transfer_mode = TRANSFER_MODE_COPY;
@@ -167,7 +168,7 @@ 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)
+ if ((old_cluster.port = strtol(optarg, &strtol_endptr, 10)) <= 0 || strtol_endptr != optarg + strlen(optarg))
{
pg_fatal("invalid old port number\n");
exit(1);
@@ -175,7 +176,7 @@ parseCommandLine(int argc, char *argv[])
break;
case 'P':
- if ((new_cluster.port = atoi(optarg)) <= 0)
+ if ((new_cluster.port = strtol(optarg, &strtol_endptr, 10)) <= 0 || strtol_endptr != optarg + strlen(optarg))
{
pg_fatal("invalid new port number\n");
exit(1);
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ae3624721e..e34786747b 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3680,6 +3680,8 @@ main(int argc, char **argv)
PGresult *res;
char *env;
+ char *strtol_endptr;
+
progname = get_progname(argv[0]);
if (argc > 1)
@@ -3737,8 +3739,8 @@ main(int argc, char **argv)
break;
case 'c':
benchmarking_option_set = true;
- nclients = atoi(optarg);
- if (nclients <= 0 || nclients > MAXCLIENTS)
+ nclients = strtol(optarg, &strtol_endptr, 10);
+ if (nclients <= 0 || nclients > MAXCLIENTS || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of clients: \"%s\"\n",
optarg);
@@ -3765,8 +3767,8 @@ main(int argc, char **argv)
break;
case 'j': /* jobs */
benchmarking_option_set = true;
- nthreads = atoi(optarg);
- if (nthreads <= 0)
+ nthreads = strtol(optarg, &strtol_endptr, 10);
+ if (nthreads <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of threads: \"%s\"\n",
optarg);
@@ -3791,8 +3793,8 @@ main(int argc, char **argv)
break;
case 's':
scale_given = true;
- scale = atoi(optarg);
- if (scale <= 0)
+ scale = strtol(optarg, &strtol_endptr, 10);
+ if (scale <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid scaling factor: \"%s\"\n", optarg);
exit(1);
@@ -3805,8 +3807,8 @@ main(int argc, char **argv)
fprintf(stderr, "specify either a number of transactions (-t) or a duration (-T), not both\n");
exit(1);
}
- nxacts = atoi(optarg);
- if (nxacts <= 0)
+ nxacts = strtol(optarg, &strtol_endptr, 10);
+ if (nxacts <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of transactions: \"%s\"\n",
optarg);
@@ -3820,8 +3822,8 @@ main(int argc, char **argv)
fprintf(stderr, "specify either a number of transactions (-t) or a duration (-T), not both\n");
exit(1);
}
- duration = atoi(optarg);
- if (duration <= 0)
+ duration = strtol(optarg, &strtol_endptr, 10);
+ if (duration <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid duration: \"%s\"\n", optarg);
exit(1);
@@ -3887,8 +3889,8 @@ main(int argc, char **argv)
break;
case 'F':
initialization_option_set = true;
- fillfactor = atoi(optarg);
- if (fillfactor < 10 || fillfactor > 100)
+ fillfactor = strtol(optarg, &strtol_endptr, 10);
+ if (fillfactor < 10 || fillfactor > 100 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid fillfactor: \"%s\"\n", optarg);
exit(1);
@@ -3913,8 +3915,8 @@ main(int argc, char **argv)
break;
case 'P':
benchmarking_option_set = true;
- progress = atoi(optarg);
- if (progress <= 0)
+ progress = strtol(optarg, &strtol_endptr, 10);
+ if (progress <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid thread progress delay: \"%s\"\n",
optarg);
@@ -3975,8 +3977,8 @@ main(int argc, char **argv)
break;
case 5:
benchmarking_option_set = true;
- agg_interval = atoi(optarg);
- if (agg_interval <= 0)
+ agg_interval = strtol(optarg, &strtol_endptr, 10);
+ if (agg_interval <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of seconds for aggregation: \"%s\"\n",
optarg);
--
2.11.0
signature.asc
Description: This is a digitally signed message part.
