Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_resetwal: Use separate flags for whether an option is given

  2. pg_resetwal: Reject negative and out of range arguments

  3. Fix pg_upgrade around multixid and mxoff wraparound

  1. Tighten up range checks for pg_resetwal arguments

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-12-03T19:07:14Z

    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

  2. Re: Tighten up range checks for pg_resetwal arguments

    Chao Li <li.evan.chao@gmail.com> — 2025-12-04T01:08:53Z

    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.

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/

  3. Re: Tighten up range checks for pg_resetwal arguments

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-12-05T18:09:37Z

    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

  4. Re: Tighten up range checks for pg_resetwal arguments

    Andrey Borodin <x4mmm@yandex-team.ru> — 2025-12-05T18:49:30Z

    
> On 4 Dec 2025, at 00:07, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 
> I got a little annoyed by how lax pg_resetwal is about out-of-range values.

A little bit offtopic, but anyway.

It's kind of common practice for many tools.
We actually had a corruption after pg_upgrade inflicted by the bug in upgrade script.
Here's the bugfix:
        exe(
            '/usr/bin/timeout 300 '
-            'sudo -u postgres /usr/lib/postgresql/{version_to}/bin/vacuumdb  --port {port}'
+            'sudo -u postgres /usr/lib/postgresql/{version_to}/bin/vacuumdb  --port {port} '
            '--analyze-in-stages --all -j 8',
            context={'port': 7432},
            allow_fail=True,
        )

Absence of space was ignored by vacuumdb. Executed command:

  vacuumdb --port 7432--analyze-in-stages

was expected to analyze only, but made a vacuum. That was not rsynced later.

So +1 from me on the strict parsing of arguments by sharp tools like pg_resetwal.


Best regards, Andrey Borodin.

  5. Re: Tighten up range checks for pg_resetwal arguments

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-12-08T14:56:22Z

    On 05/12/2025 20:09, Heikki Linnakangas wrote:
> On 04/12/2025 03:08, Chao Li wrote:
>> 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().

I think it's best to reject the "-0" case, so I kept the code so that 
it's rejected, and added a test for that.

Committed, thanks for the review!

- Heikki