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. 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/
