Hi all,
On Tue, Oct 13, 2026 at 12:46 PM Egor Ivkov <[email protected]> wrote: > > > 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, > Thank you for the updated patch. I tested the v2 patch on the current development branch. The patch builds successfully, and I verified the reported issue using a reproduction test with an oversized tablespace OID (4294967297). On the unpatched source, pg_combinebackup failed with a missing tablespace mapping error, consistent with the oversized OID being incorrectly accepted. With the v2 patch applied, the utility correctly skipped the invalid symbolic link and completed successfully, issuing the following warning: pg_combinebackup: warning: skipping symbolic link ".../pg_tblspc/4294967297". The reproduction test confirmed the expected difference in behavior between the unpatched and patched versions. I also ran the complete pg_combinebackup TAP test suite, and all tests passed. Based on my testing and review, I did not find any issues with the patch. Overall, the patch looks good to me. Regards, Solai
