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_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