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

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