pg_resetwal: Reject negative and out of range arguments

The strtoul() function that we used to parse many of the options
accepts negative values, and silently wraps them to the equivalent
unsigned values. For example, -1 becomes 0xFFFFFFFF, on platforms
where unsigned long is 32 bits wide. Also, on platforms where
"unsigned long" is 64 bits wide, we silently casted values larger than
UINT32_MAX to the equivalent 32-bit value. Both of those behaviors
seem undesirable, so tighten up the parsing to reject them.

Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://www.postgresql.org/message-id/81adf5f3-36ad-4bcd-9ba5-1b95c7b7a807@iki.fi

2 files changed, 78 insertions, 18 deletions

diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index 8d5d9805279..8ca8dad01a0 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -92,6 +92,7 @@ static void KillExistingArchiveStatus(void);
 static void KillExistingWALSummaries(void);
 static void WriteEmptyXLOG(void);
 static void usage(void);
+static uint32 strtouint32_strict(const char *restrict s, char **restrict endptr, int base);
 
 
 int
@@ -120,7 +121,6 @@ main(int argc, char *argv[])
 	MultiXactId set_oldestmxid = 0;
 	char	   *endptr;
 	char	   *endptr2;
-	int64		tmpi64;
 	char	   *DataDir = NULL;
 	char	   *log_fname = NULL;
 	int			fd;
@@ -162,7 +162,7 @@ main(int argc, char *argv[])
 
 			case 'e':
 				errno = 0;
-				set_xid_epoch = strtoul(optarg, &endptr, 0);
+				set_xid_epoch = strtouint32_strict(optarg, &endptr, 0);
 				if (endptr == optarg || *endptr != '\0' || errno != 0)
 				{
 					/*------
@@ -177,7 +177,7 @@ main(int argc, char *argv[])
 
 			case 'u':
 				errno = 0;
-				set_oldest_xid = strtoul(optarg, &endptr, 0);
+				set_oldest_xid = strtouint32_strict(optarg, &endptr, 0);
 				if (endptr == optarg || *endptr != '\0' || errno != 0)
 				{
 					pg_log_error("invalid argument for option %s", "-u");
@@ -190,7 +190,7 @@ main(int argc, char *argv[])
 
 			case 'x':
 				errno = 0;
-				set_xid = strtoul(optarg, &endptr, 0);
+				set_xid = strtouint32_strict(optarg, &endptr, 0);
 				if (endptr == optarg || *endptr != '\0' || errno != 0)
 				{
 					pg_log_error("invalid argument for option %s", "-x");
@@ -203,7 +203,7 @@ main(int argc, char *argv[])
 
 			case 'c':
 				errno = 0;
-				set_oldest_commit_ts_xid = strtoul(optarg, &endptr, 0);
+				set_oldest_commit_ts_xid = strtouint32_strict(optarg, &endptr, 0);
 				if (endptr == optarg || *endptr != ',' || errno != 0)
 				{
 					pg_log_error("invalid argument for option %s", "-c");
@@ -229,7 +229,7 @@ main(int argc, char *argv[])
 
 			case 'o':
 				errno = 0;
-				set_oid = strtoul(optarg, &endptr, 0);
+				set_oid = strtouint32_strict(optarg, &endptr, 0);
 				if (endptr == optarg || *endptr != '\0' || errno != 0)
 				{
 					pg_log_error("invalid argument for option %s", "-o");
@@ -242,7 +242,7 @@ main(int argc, char *argv[])
 
 			case 'm':
 				errno = 0;
-				set_mxid = strtoul(optarg, &endptr, 0);
+				set_mxid = strtouint32_strict(optarg, &endptr, 0);
 				if (endptr == optarg || *endptr != ',' || errno != 0)
 				{
 					pg_log_error("invalid argument for option %s", "-m");
@@ -250,7 +250,7 @@ main(int argc, char *argv[])
 					exit(1);
 				}
 
-				set_oldestmxid = strtoul(endptr + 1, &endptr2, 0);
+				set_oldestmxid = strtouint32_strict(endptr + 1, &endptr2, 0);
 				if (endptr2 == endptr + 1 || *endptr2 != '\0' || errno != 0)
 				{
 					pg_log_error("invalid argument for option %s", "-m");
@@ -269,17 +269,13 @@ main(int argc, char *argv[])
 
 			case 'O':
 				errno = 0;
-				tmpi64 = strtoi64(optarg, &endptr, 0);
+				set_mxoff = strtouint32_strict(optarg, &endptr, 0);
 				if (endptr == optarg || *endptr != '\0' || errno != 0)
 				{
 					pg_log_error("invalid argument for option %s", "-O");
 					pg_log_error_hint("Try \"%s --help\" for more information.", progname);
 					exit(1);
 				}
-				if (tmpi64 < 0 || tmpi64 > (int64) MaxMultiXactOffset)
-					pg_fatal("multitransaction offset (-O) must be between 0 and %u", MaxMultiXactOffset);
-
-				set_mxoff = (MultiXactOffset) tmpi64;
 				mxoff_given = true;
 				break;
 
@@ -1214,3 +1210,45 @@ usage(void)
 	printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT);
 	printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
 }
+
+/*
+ * strtouint32_strict -- like strtoul(), but returns uint32 and doesn't accept
+ * negative values
+ */
+static uint32
+strtouint32_strict(const char *restrict s, char **restrict endptr, int base)
+{
+	unsigned long val;
+	bool		is_neg;
+
+	/* skip leading whitespace */
+	while (isspace(*s))
+		s++;
+
+	/*
+	 * Is it negative?  We still call strtoul() if it was, to set 'endptr'.
+	 * (The current callers don't care though.)
+	 */
+	is_neg = (*s == '-');
+
+	val = strtoul(s, endptr, base);
+
+	/* reject if it was negative */
+	if (errno == 0 && is_neg)
+	{
+		errno = ERANGE;
+		val = 0;
+	}
+
+	/*
+	 * reject values larger than UINT32_MAX on platforms where long is 64 bits
+	 * wide.
+	 */
+	if (errno == 0 && val != (uint32) val)
+	{
+		errno = ERANGE;
+		val = UINT32_MAX;
+	}
+
+	return (uint32) val;
+}
diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl
index 90ecb8afe18..8717b144bc0 100644
--- a/src/bin/pg_resetwal/t/001_basic.pl
+++ b/src/bin/pg_resetwal/t/001_basic.pl
@@ -103,7 +103,7 @@ command_fails_like(
 	'fails with incorrect -e option');
 command_fails_like(
 	[ 'pg_resetwal', '-e' => '-1', $node->data_dir ],
-	qr/must not be -1/,
+	qr/error: invalid argument for option -e/,
 	'fails with -e value -1');
 # -l
 command_fails_like(
@@ -122,13 +122,20 @@ command_fails_like(
 
 # This used to be forbidden, but nextMulti can legitimately be 0 after
 # wraparound, so we now accept it in pg_resetwal too.
-command_ok([ 'pg_resetwal', '-m' => '0,10', $node->data_dir ],
-	'succeeds with -m value 0 part 1');
+command_ok(
+	[ 'pg_resetwal', '-m' => '0,10', $node->data_dir ],
+	'succeeds with -m value 0 in the first part');
+
+# -0 doesn't make sense however
+command_fails_like(
+	[ 'pg_resetwal', '-m' => '-0,10', $node->data_dir ],
+	qr/error: invalid argument for option -m/,
+	'fails with -m value -0 in the first part');
 
 command_fails_like(
 	[ 'pg_resetwal', '-m' => '10,0', $node->data_dir ],
 	qr/must not be 0/,
-	'fails with -m value 0 part 2');
+	'fails with -m value 0 in the second part');
 # -o
 command_fails_like(
 	[ 'pg_resetwal', '-o' => 'foo', $node->data_dir ],
@@ -145,7 +152,7 @@ command_fails_like(
 	'fails with incorrect -O option');
 command_fails_like(
 	[ 'pg_resetwal', '-O' => '-1', $node->data_dir ],
-	qr/must be between 0 and 4294967295/,
+	qr/error: invalid argument for option -O/,
 	'fails with -O value -1');
 # --wal-segsize
 command_fails_like(
@@ -175,6 +182,21 @@ command_fails_like(
 	qr/must be greater than/,
 	'fails with -x value too small');
 
+# Check out of range values with -x. These are forbidden for all other
+# 32-bit values too, but we use just -x to exercise the parsing.
+command_fails_like(
+	[ 'pg_resetwal', '-x' => '-1', $node->data_dir ],
+	qr/error: invalid argument for option -x/,
+	'fails with -x value -1');
+command_fails_like(
+	[ 'pg_resetwal', '-x' => '-100', $node->data_dir ],
+	qr/error: invalid argument for option -x/,
+	'fails with negative -x value');
+command_fails_like(
+	[ 'pg_resetwal', '-x' => '10000000000', $node->data_dir ],
+	qr/error: invalid argument for option -x/,
+	'fails with -x value too large');
+
 # --char-signedness
 command_fails_like(
 	[ 'pg_resetwal', '--char-signedness', 'foo', $node->data_dir ],