> I still have one concern about the implementation. On some platforms,
> unsigned long is also 32 bits, so the fix would still accept -1 there.
> Maybe we can use the same approach here.
You're right, thanks I confirmed it. Fixed the same way as in parse_relfilenumber().
Attached v2 patch.
Regards,
Egor Ivkov
On Sep 23, 2026, at 04:10, Egor Ivkov <[email protected]> wrote:
Hi,
parse_oid() in pg_combinebackup assigns the result of strtoul() to an Oid
before range-checking it:
Oid oid;
...
oid = strtoul(s, &ep, 10);
if (errno != 0 || *ep != '\0' || oid < 1 || oid > PG_UINT32_MAX)
return false;
Since Oid is 32 bits, the value has already been truncated by the time
"oid > PG_UINT32_MAX" is evaluated, so on platforms where unsigned long is
wider than 32 bits that test can never fire. An out-of-range string is
then accepted as its truncated value rather than being rejected:
"4294967297" is accepted as OID 1, and "-1" is accepted as OID 4294967295.
parse_oid() is only fed directory names found under pg_tblspc, so the
practical consequence is limited: pg_combinebackup treats a bogus
directory name as a valid tablespace OID instead of ignoring it. It still
seems worth fixing.
The attached patch keeps the parsed value in an unsigned long until it has
been checked and casts to Oid afterwards, matching what
parse_relfilenumber() in pg_upgrade already does.
The patch is against master. The same code is present unchanged back to
v17 (dc212340058), and the patch applies cleanly to REL_17_STABLE,
REL_18_STABLE and REL_19_STABLE.
Regards,
Egor Ivkov<v1-0001-pg_combinebackup-make-the-OID-range-check-in-pars.patch>--
+1 on the direction.
I still have one concern about the implementation. On some platforms, unsigned long is also 32 bits, so the fix would still accept -1 there.
I see that parse_relfilenumber() checks the first character before calling strtoul():
```
static RelFileNumber
parse_relfilenumber(const char *filename)
{
char *endp;
unsigned long n;
if (filename[0] < '1' || filename[0] > '9')
return InvalidRelFileNumber;
errno = 0;
n = strtoul(filename, &endp, 10);
if (errno || filename == endp || n <= 0 || n > PG_UINT32_MAX)
return InvalidRelFileNumber;
return (RelFileNumber) n;
}
```
Maybe we can use the same approach here.
Best regards,
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Egor Ivkov <[email protected]> Date: Tue, 22 Sep 2026 22:44:05 +0300 Subject: [PATCH v2] pg_combinebackup: make the OID range check in parse_oid() effective
parse_oid() assigned the result of strtoul() to an Oid variable before
range-checking it, so the value had already been truncated to 32 bits by
the time "oid > PG_UINT32_MAX" was evaluated. On platforms where
unsigned long is wider than 32 bits that test is dead code, and an
out-of-range string is accepted as its truncated value rather than being
rejected: "4294967297" is accepted as OID 1. Keep the parsed value in an
unsigned long until it has been checked, and cast to Oid afterwards.
Also reject up front any string that does not start with a digit between
1 and 9. strtoul() accepts leading whitespace, an explicit sign and
leading zeroes, for exmaple "-1": strtoul() negates rather than reporting
ERANGE, so on platforms where unsigned long is only 32 bits it survives
the range check too and is accepted as OID 4294967295. Checking the
first character rejects all of these regardless of the width of unsigned
long.
Both changes mirror parse_relfilenumber() in pg_upgrade, which handles
the same problem correctly.
parse_oid() is only fed directory names found under pg_tblspc, so the
practical consequence is limited to pg_combinebackup treating a bogus
directory name as a valid tablespace OID instead of ignoring it. Both
call sites go through parse_oid(), so they remain consistent with each
other.
---
src/bin/pg_combinebackup/pg_combinebackup.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index 254a27b125b..03cc75f5728 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -816,15 +816,18 @@ help(const char *progname)
static bool
parse_oid(char *s, Oid *result)
{
- Oid oid;
+ unsigned long oid;
char *ep;
+ if (s[0] < '1' || s[0] > '9')
+ return false;
+
errno = 0;
oid = strtoul(s, &ep, 10);
if (errno != 0 || *ep != '\0' || oid < 1 || oid > PG_UINT32_MAX)
return false;
- *result = oid;
+ *result = (Oid) oid;
return true;
}
--
2.43.0
