Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ
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.
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:t253674psql -h localhost -U postgresBuilt from patchset v3 (message #3), October 24, 2026 at 06:41 PM.
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 t253674_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 t253674_3 && git checkout t253674_3Patchset v3 (message #3) is on t253674_3
Hi,
Michael reported this as BUG #19599. He wasn't sure it was a real issue, but
I think the missing check is still worth fixing. Postgres never writes a
hole that doesn't fit in the page, but DecodeXLogRecord() will happily
accept one from a corrupt or hand-built record, and RestoreBlockImage()
then uses those fields as memcpy/MemSet lengths.
What decode checks today is only non-zero values:
if ((blk->bimg_info & BKPIMAGE_HAS_HOLE) &&
(blk->hole_offset == 0 ||
blk->hole_length == 0 ||
blk->bimg_len == BLCKSZ))
It never asks whether the hole actually fits in the page. Both fields
are uint16s taken from the record, so this gets through:
hole_offset = 8000
hole_length = 8000 /* 16000 > BLCKSZ */
bimg_len = 16
bimg_info = HAS_HOLE | APPLY | COMPRESS_PGLZ
That's specifically a compressed image, because that's the only shape
where hole_length is stored in the WAL rather than computed. The CRC
can still be valid. After that, RestoreBlockImage() does:
memcpy(page, ptr, hole_offset);
MemSet(page + hole_offset, 0, hole_length);
memcpy(..., BLCKSZ - (hole_offset + hole_length));
With the numbers above, the first memcpy already runs off the end of an
8kB page, and the last length underflows to a huge size_t. The
decompressors have the same problem: they are told the output buffer is
BLCKSZ - hole_length bytes.
I've attached the patch that adds the missing bound to that existing HAS_HOLE
check:
hole_offset > BLCKSZ ||
hole_length > BLCKSZ - hole_offset
(subtraction rather than addition, so the two uint16s can't overflow.)
The same check is repeated at the start of RestoreBlockImage(), before
decompression or memcpy. A bad hole still uses the existing HAS_HOLE
error message, I didn't add a new one.
I also have a small frontend test that builds this record in memory (valid
header and CRC, compressed image, bad hole) and feeds it to
DecodeXLogRecord(). Happy to send that if it's useful, I left it out of
this mail so the patch stays small.
Does this look like the right approach?
I'd also like to hear whether it's worth back-patching.
Thanks,
Yuriy Grigoryev
Hi Yuriy,
I reviewed and tested v1. The fix looks right to me, and I think it
should go in. Some comments below.
Testing (macOS arm64, Apple clang 17, meson debug build with
assertions, master at f25c50fd8f plus v1 as cfbot applies it):
- The regression tests and the pg_walinspect tests pass.
- Crash recovery with wal_compression = pglz, lz4, zstd and off, each
replaying about 1,100 full-page images with holes, plus a pglz run
with wal_consistency_checking = all (about 21,000 images). The data
matched before and after the crash, and amcheck (verify_heapam, and
bt_index_check with heapallindexed) found nothing.
- I crafted records whose hole doesn't fit in the page (hole_offset
moved so that hole_offset + hole_length = 9192, record CRC
recomputed), once in an uncompressed FPI and once in a
pglz-compressed one. Without the patch, pg_waldump decodes both
records without complaint, and pg_waldump --save-fullpage crashes on
both (SIGBUS and SIGSEGV). With the patch, both are rejected at
decode time, and crash recovery over the same WAL stops there with
BKPIMAGE_HAS_HOLE set, but hole offset 2364 length 6828 block image
length 416 at ...
instead of writing past the page.
Comments:
1. Uncompressed images are affected too, not only compressed ones.
There, hole_length is derived as BLCKSZ - bimg_len, and nothing
checked hole_offset against bimg_len. The new condition covers that
case as well (for an uncompressed image it amounts to requiring
hole_offset <= bimg_len), so the commit message could say so; right
now it only mentions compressed images.
2. v1 doesn't apply to master with git am. The context line in
DecodeXLogRecord() still reads "hole offset %u ... at %X/%X", so I
guess it was made against an older branch. cfbot's copy applies,
but a rebased v2 would make it easier to test.
3. The new message in RestoreBlockImage() uses %X/%X, while everything
else in xlogreader.c uses %X/%08X now. Its text is also identical
to the existing message for a block without an image, so the two
failures can't be told apart in the log.
4. DecodeXLogRecord() is the only place that fills in the image fields
of DecodedBkpBlock, so with this patch the re-check in
RestoreBlockImage() can't fail; in my tests it never did. I'd make
it an Assert(). If you prefer a runtime check, a distinct message
would help (see 3).
About back-patching: XLogRecordAssemble() only creates a hole when
pd_lower >= SizeOfPageHeaderData, pd_upper > pd_lower and
pd_upper <= BLCKSZ, so hole_offset + hole_length <= BLCKSZ for any WAL
that PostgreSQL writes. The new check can't reject valid WAL, and it
turns memory corruption on a bad record into a clean error. So +1 for
back-patching from me.
Unrelated to this patch, but noticed while testing: the error messages
in DecodeXLogRecord() print state->ReadRecPtr rather than the lsn of
the record being decoded. With read-ahead that's an earlier record:
pg_waldump reported the corrupt record at 0/0197B3A0 (the previous
record) instead of 0/0197B7E8, and in recovery the reported LSN was two
records back. I can send a separate patch for that.
It would still be good to see the frontend test you mentioned. I can
also share the script I used to craft the records if that helps.
Regards,
Rahul Yadav
Hi Rahul,
Thank you very much for the thorough review and testing -- the crash-recovery matrix (pglz/lz4/zstd/off + consistency checking) and especially the crafted-record repro with pg_waldump --save-fullpage crashing without the patch are super convincing. And thanks for confirming the back-patch safety argument.
v2 attached, addressing all your points:
Commit message now says uncompressed images are affected too. You are right -- there hole_length is derived as BLCKSZ - bimg_len and hole_offset was unchecked against bimg_len; the new bound covers it (it amounts to hole_offset <= bimg_len there).
Rebased onto current master (%d / %X/%08X). Sorry for the stale context in v1 -- it was made against an older tree. v2 applies with git am.
3-4. Dropped the runtime re-check in RestoreBlockImage() in favor of Assert()s, as you suggested. Since DecodeXLogRecord() is the only filler of DecodedBkpBlock, it cannot fail, and this also resolves the wrong format and the duplicate message text.
About back-patching: agreed, thanks for spelling out the XLogRecordAssemble() condition -- no valid WAL can trip the new check.
About the ReadRecPtr vs decoded-record LSN issue: good catch, thank you for noticing while testing. And yes, please send that as a separate patch.
About the frontend test: attached as 0002, since you said you'd like to see it. It is a small src/test/modules/test_xlogreader harness that builds hole-image records in memory (valid CRC, compressed + uncompressed) and feeds them to DecodeXLogRecord(), so it checks rejection happens at decode time, before RestoreBlockImage() can touch the page:
- valid compressed hole, hole ending exactly at BLCKSZ,
- the BUG #19599 shape (hole_offset + hole_length > BLCKSZ), one byte past the page, hole_offset past the page,
- valid uncompressed hole and uncompressed hole_offset past the image.
With v2 all valid cases are accepted and all malformed ones are rejected with the existing BKPIMAGE_HAS_HOLE set message. I kept it as a separate patch to keep v2 small -- happy to squash if you prefer. And I'd still love to see your crafting script, it sounds more end-to-end than mine.
Thanks again,
Yuriy
Attachments:
t253674_30001-Reject-WAL-block-images-whose-hole-does-not-fit-v2.patchapplication/octet-stream; name=0001-Reject-WAL-block-images-whose-hole-does-not-fit-v2.patchDownload+17-4
0002-Add-test_xlogreader-for-malformed-FPI-hole-geometry.patchapplication/octet-stream; name=0002-Add-test_xlogreader-for-malformed-FPI-hole-geometry.patchDownload+345-1
Hi Yuriy,
Thanks for v2. All my comments are addressed, and I'm marking the
entry Ready for Committer.
Testing (macOS arm64, meson debug build with assertions, master at
9510a826e4 plus v2):
- Both patches apply cleanly with git am, build without warnings, and
the regression tests pass.
- test_xlogreader passes. Without 0001 it fails the four malformed
cases and still accepts the three valid ones, so it does catch the
bug.
- Crash recovery with wal_compression = off, pglz, lz4 and zstd, each
replaying about 1,000 full-page images with holes, plus a pglz run
with wal_consistency_checking = all (about 3,500 images). The data
matched before and after the crash, amcheck found nothing, and the
new Asserts never fired.
- Records with the hole moved past the page, one uncompressed and one
pglz-compressed: pg_waldump reports the BKPIMAGE_HAS_HOLE error,
pg_waldump --save-fullpage exits with an error instead of crashing,
and crash recovery stops at the bad record.
One small suggestion for 0002: the uncompressed cases don't test the
boundary. hole_offset == bimg_len (accepted, the hole ends exactly at
BLCKSZ) and hole_offset == bimg_len + 1 (rejected) would match what
the compressed cases already do.
The same runs show the LSN problem again: for the corrupted record at
0/01E582A0, pg_waldump reported 0/01E58218, the previous record, and
recovery reported 0/01E581B8. I'll send that patch in a separate
thread.
The crafting script is attached. It finds the first full-page image
with a hole after a given LSN in a copy of pg_wal, moves the hole past
the end of the page, and recomputes the record CRC.
Regards,
Rahul Yadav
Attachments:
corrupt_fpi.pytext/plain; charset=US-ASCII; name=corrupt_fpi.pyDownload
On Tue, Sep 29, 2026 at 08:37:02AM +0000, Grigorev Jurij wrote:
Thank you very much for the thorough review and testing -- the
crash-recovery matrix (pglz/lz4/zstd/off + consistency checking) and
especially the crafted-record repro with pg_waldump --save-fullpage
crashing without the patch are super convincing. And thanks for
confirming the back-patch safety argument.v2 attached, addressing all your points:
XLogRecordAssemble() in xloginsert.c enforces a size policy already
when a page image needs to be included in a record (REGBUF_STANDARD
case, for both the "lower" and "upper" cases). The argument of a
corrupted record does not stand, a CRC32 check would complain before
we ever reach this path. The hand-made record record argument is also
something I have a hard time to buy, because WAL data is trusted.
So, I don't understand what this patch buys us at all, except more
complexity in the replay path.
There may be an argument for the xlogreader facility, but this relies
on the premise that incorrect WAL records are a thing out there.
Again here comes the CRC check in the record header and the WAL
insertion enforcing already some bounds. This feels like test bloat
to me.
--
Michael
Hi Michael,
Thanks for looking! I agree XLogRecordAssemble() never writes
such a hole, CRC catches accidental corruption, and core WAL is
trusted.
My only point was that DecodeXLogRecord() already distrusts these
fields enough to cross-check them: it rejects hole_offset == 0,
hole_length == 0, bimg_len == BLCKSZ when HAS_HOLE is set, and
non-zero hole fields when it is not set. Bounding
hole_offset + hole_length against BLCKSZ just completes that
existing family of checks. Rahul's repro shows a re-CRCed record
still passes decode and then crashes pg_waldump --save-fullpage.
I agree the test in 0002 is quite large for such a small check --
happy to drop it entirely. To keep this minimal, we could keep
just the two-line check in DecodeXLogRecord() with no extra test --
the existing HAS_HOLE error message, no new paths.
I don't insist on the test or backpatch -- if you prefer, let's
keep only the decode check, or close it if you think even that
is not wanted. Should xlogreader be robust here, or may
RestoreBlockImage() assume trusted input after CRC?
I understand from your message that you lean towards this not
being needed, given trusted WAL, CRC and the insertion bounds --
just wanted to understand where the line is. Happy to update
or close as you suggest.
Kind regards,
Yuriy