Re: [PATCH] pg_combinebackup: make the OID range check in parse_oid() effective

> On Sep 23, 2026, at 04:10, Egor Ivkov <e(dot)ivkov(at)arenadata(dot)io> 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/