Backup manifests accept out-of-range LSNs
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253315psql -h localhost -U postgresBuilt from patchset v3 (message #3), September 04, 2026 at 10:48 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253315_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253315_3 && git checkout t253315_3Patchset v3 (message #3) is on t253315_3
Hi,
355814931141 tightened the integer fields of a backup manifest, but
parse_xlogrecptr() in the same file still does:
if (sscanf(input, "%X/%08X", &hi, &lo) != 2)
"%X" stores into a uint32, so an oversized half is accepted with its
high bits dropped, and trailing text is ignored, since sscanf() still
returns 2. "123456789/0" is read as 23456789/0 and "1/2garbage" as
1/2, while "oops" is rejected as before.
That value validates the WAL ranges of a backup, in pg_verifybackup,
pg_combinebackup and the server's incremental backup path, so those
checks end up using a location the manifest doesn't contain.
The attached requires one to eight hex digits per half and the whole
string consumed, matching pg_lsn_in_safe(), and adds a few cases to
005_bad_manifest.pl. I stayed away from the strtoul()/endptr idiom the
neighbouring parsers use, since strtoul("-1", ..., 16) returns
ULONG_MAX without setting ERANGE, so "-1/0" would slip through as
FFFFFFFF/0.
Thoughts?
Regards,
Ayush
Reviewed this patch.
The issue seems valid. PostgreSQL-generated manifests should not normally
contain such LSNs, but a malformed or modified manifest could make the
parser operate on a different LSN than the one specified. The current
sscanf() based parser allows this through truncation of oversized
components and ignored trailing characters.
The proposed fix looks good and the tests cover the regression cases.
Show quoted text
Hi,
On Thu, 6 Aug 2026 at 07:22, Yuefei Shi <shiyuefei1004@gmail.com> wrote:
Reviewed this patch.
The issue seems valid. PostgreSQL-generated manifests should not normally
contain such LSNs, but a malformed or modified manifest could make the
parser operate on a different LSN than the one specified. The current
sscanf() based parser allows this through truncation of oversized
components and ignored trailing characters.The proposed fix looks good and the tests cover the regression cases.
Thanks for the review!
Attaching v2 which uses the new pg_parse_lsn funct.
Regards,
Ayush
On Wed, Aug 12, 2026 at 6:27 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:
Attaching v2 which uses the new pg_parse_lsn funct.
Thanks for updating the patch! LGTM.
Barring any objections, I will commit the patch.
Regards,
--
Fujii Masao