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 <[email protected]> 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
