Tighten up range checks for pg_resetwal arguments

Hi Heikki,

This patch looks like a straightforward change, but I see a correctness issue and a few small comments.

On Dec 4, 2025, at 03:07, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

While working on the 64-bit multixid offsets patch and commit 94939c5f3a, I got a little annoyed by how lax pg_resetwal is about out-of-range values. These are currently accepted, for example:

# Negative XID
pg_resetwal -D data -x -1000

# XID larger than 2^32 (on some platforms)
pg_resetwal -D data -x 10000000000

The first attached patch tightens up the parsing to reject those.

The second attached patch is just refactoring. Currently, we use invalid values for the variables backing each of the options to mean "option was not given". I think it would be more clear to have separate boolean variables for that. I did that for the --multixact-ids option in commit f99e30149f already, because there was no unused value for multixid that we could use. This patch expands that to all the options.

- Heikki
<0001-pg_resetwal-Reject-negative-and-out-of-range-argumen.patch><0002-pg_resetwal-Use-separate-flags-for-whether-an-option.patch>

1 - 0002 - correctness issue
```
-	if (set_oid != 0)
-		ControlFile.checkPointCopy.nextOid = set_oid;
+	if (next_oid_given)
+		ControlFile.checkPointCopy.nextOid = next_oid_val;
```

As OID 0 is invalid, the old code checks that. But the new code checks only next_oid_given, which loses the validation of invalid OID 0.

This issue applies to multiple parameters.

2 - 0001
```
+/*
+ * 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;
+}
```

I guess this function doesn’t have to check “-“ by itself, it leads some edge-case not to be well handled, for example “-0” is still 0, not a negative value. We can use strtoll() convert input string to a singed long long, and check if value is negative.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On 04/12/2025 03:08, Chao Li wrote:

Hi Heikki,

This patch looks like a straightforward change, but I see a correctness issue and a few small comments.

On Dec 4, 2025, at 03:07, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

While working on the 64-bit multixid offsets patch and commit 94939c5f3a, I got a little annoyed by how lax pg_resetwal is about out-of-range values. These are currently accepted, for example:

# Negative XID
pg_resetwal -D data -x -1000

# XID larger than 2^32 (on some platforms)
pg_resetwal -D data -x 10000000000

The first attached patch tightens up the parsing to reject those.

The second attached patch is just refactoring. Currently, we use invalid values for the variables backing each of the options to mean "option was not given". I think it would be more clear to have separate boolean variables for that. I did that for the --multixact-ids option in commit f99e30149f already, because there was no unused value for multixid that we could use. This patch expands that to all the options.

- Heikki
<0001-pg_resetwal-Reject-negative-and-out-of-range-argumen.patch><0002-pg_resetwal-Use-separate-flags-for-whether-an-option.patch>

1 - 0002 - correctness issue
```
-	if (set_oid != 0)
-		ControlFile.checkPointCopy.nextOid = set_oid;
+	if (next_oid_given)
+		ControlFile.checkPointCopy.nextOid = next_oid_val;
```

As OID 0 is invalid, the old code checks that. But the new code checks only next_oid_given, which loses the validation of invalid OID 0.

This issue applies to multiple parameters.

There's this check earlier:

case 'o':
errno = 0;
next_oid_val = 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 (next_oid_val == 0)
pg_fatal("OID (-o) must not be 0");
next_oid_given = true;
break;

That's covered by the tap test too.

2 - 0001
```
+/*
+ * 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;
+}
```

I guess this function doesn’t have to check “-“ by itself, it leads some edge-case not to be well handled, for example “-0” is still 0, not a negative value. We can use strtoll() convert input string to a singed long long, and check if value is negative.

True. I originally wrote this for the 64-bit variant which will be used
in the 64-bit offsets patch. For that we can't use strtoll().

Thanks for the review!

- Heikki

On 4 Dec 2025, at 00:07, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

I got a little annoyed by how lax pg_resetwal is about out-of-range values.

A little bit offtopic, but anyway.

It's kind of common practice for many tools.
We actually had a corruption after pg_upgrade inflicted by the bug in upgrade script.
Here's the bugfix:
        exe(
            '/usr/bin/timeout 300 '
-            'sudo -u postgres /usr/lib/postgresql/{version_to}/bin/vacuumdb  --port {port}'
+            'sudo -u postgres /usr/lib/postgresql/{version_to}/bin/vacuumdb  --port {port} '
            '--analyze-in-stages --all -j 8',
            context={'port': 7432},
            allow_fail=True,
        )

Absence of space was ignored by vacuumdb. Executed command:

vacuumdb --port 7432--analyze-in-stages

was expected to analyze only, but made a vacuum. That was not rsynced later.

So +1 from me on the strict parsing of arguments by sharp tools like pg_resetwal.

Best regards, Andrey Borodin.

On 05/12/2025 20:09, Heikki Linnakangas wrote:

On 04/12/2025 03:08, Chao Li wrote:

I guess this function doesn’t have to check “-“ by itself, it leads
some edge-case not to be well handled, for example “-0” is still 0,
not a negative value. We can use strtoll() convert input string to a
singed long long, and check if value is negative.

True. I originally wrote this for the 64-bit variant which will be used
in the 64-bit offsets patch. For that we can't use strtoll().

I think it's best to reject the "-0" case, so I kept the code so that
it's rejected, and added a test for that.

Committed, thanks for the review!

- Heikki