Subject: [PATCH] Port most calls of atoi(optarg) to strcol

From: Pierre Ducroquet <[email-masked]>
Date: Wed, 13 Sep 2017 19:51:09 +0200

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                |  9 ++++++++-
 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              | 33 +++++++++++++++++----------------
 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 51509d150e..b51b62cc21 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;
@@ -2072,6 +2072,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'},
@@ -2212,8 +2213,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);
@@ -2251,8 +2252,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 710a33ab4d..c1651961b5 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -494,6 +494,7 @@ main(int argc, char **argv)
 	int			c;
 	int			option_index;
 	char	   *db_name;
+	char	   *strtol_endptr = NULL;
 	uint32		hi, lo;
 
 	progname = get_progname(argv[0]);
@@ -529,7 +530,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);
@@ -547,8 +548,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);
@@ -575,8 +576,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 6811a55e76..bde45b2650 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 = NULL;
 
 	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 4e02c4cea1..2190412293 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2100,6 +2100,7 @@ main(int argc, char **argv)
 	};
 
 	char	   *env_wait;
+	char	   *strtol_endptr = NULL;
 	int			option_index;
 	int			c;
 	pgpid_t		killproc = 0;
@@ -2231,7 +2232,13 @@ 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 75f08cd792..ddbf742878 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -290,6 +290,7 @@ main(int argc, char **argv)
 	const char *dumpencoding = NULL;
 	const char *dumpsnapshot = NULL;
 	char	   *use_role = NULL;
+	char	   *strtol_endptr = NULL;
 	int			numWorkers = 1;
 	trivalue	prompt_password = TRI_DEFAULT;
 	int			compressLevel = -1;
@@ -441,7 +442,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) */
@@ -507,8 +513,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 860a211a3c..e759a6e960 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -65,6 +65,7 @@ main(int argc, char **argv)
 	int			numWorkers = 1;
 	Archive    *AH;
 	char	   *inputFileSpec;
+	char	   *strtol_endptr = NULL;
 	static int	disable_triggers = 0;
 	static int	enable_row_security = 0;
 	static int	if_exists = 0;
@@ -181,7 +182,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 c74eb25e18..678b5a8bfa 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 = NULL;
 	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 e37496c971..27746bcd89 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3708,6 +3708,7 @@ main(int argc, char **argv)
 	PGconn	   *con;
 	PGresult   *res;
 	char	   *env;
+	char	   *strtol_endptr;
 
 	progname = get_progname(argv[0]);
 
@@ -3766,8 +3767,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);
@@ -3794,8 +3795,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);
@@ -3820,8 +3821,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);
@@ -3834,8 +3835,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);
@@ -3849,8 +3850,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);
@@ -3916,8 +3917,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);
@@ -3937,8 +3938,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);
@@ -3999,8 +4000,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.14.1

--nextPart11492799.g3MEUzi5y5
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0

-- 
Sent via pgsql-hackers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--nextPart11492799.g3MEUzi5y5--