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@iki.fi> wrote:
>>
>> While working on the 64-bit multixid offsets patch and commit 94939c5f3a, I got a little annoyed by how lax
pg_resetwalis 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
theoptions to mean "option was not given". I think it would be more clear to have separate boolean variables for that.
Idid that for the --multixact-ids option in commit f99e30149f already, because there was no unused value for multixid
thatwe 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
validationof 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
checkif 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