Re: Tighten up range checks for pg_resetwal arguments

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(at)iki(dot)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