agora inbox for pgsql-bugs@postgresql.org
help / color / mirror / Atom feedBUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ
7+ messages / 4 participants
[nested] [flat]
* BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ
@ 2026-08-02 17:51 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 0 replies; 7+ messages in thread
From: PG Bug reporting form @ 2026-08-02 17:51 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: malis@pgrust.com
The following bug has been logged on the website:
Bug reference: 19599
Logged by: Michael Malis
Email address: malis@pgrust.com
PostgreSQL version: 18.3
Operating system: Debian
Description:
I don't think this is a real issue because it requires a specific WAL
structure, but I figured I would report it anyway.
DecodeXLogRecord cross-checks a block image's hole descriptor for
non-zero-ness only. It never checks that the hole fits inside the page. A
record that sets both hole_offset and hole_length large — each is a uint16,
so up to 65535 — passes validation, and RestoreBlockImage then uses those
values directly as memcpy/MemSet offsets and lengths into an 8 kB page
buffer.
Evidence status — please read
-----------------------------
- What we verified: the missing bound, by reading 18.3. Line numbers below.
- What we did NOT do: execute the overrun. Our differential harness detects
the out-of-bounds geometry and skips the C oracle for those inputs
precisely so it never drives undefined behaviour, so no ASan/valgrind
report exists and we cannot attach one. We also have no live-server
reproducer, because constructing the record requires authoring WAL with a
valid CRC over the malformed body — our builder does this inside the
harness, not against a server.
- What is positively demonstrated: our reimplementation rejects this
geometry, and that rejection is asserted under fuzzing (one-sided). That
is evidence the input class is reachable through decode, not evidence
about C's behaviour.
We would rather file this as "here is a missing check, here is the input
that reaches it" than overstate it. If you want the overrun demonstrated
under a sanitizer before considering it, that is a reasonable ask and we can
do it.
Reproducer (harness-level)
--------------------------
A WAL record whose block-image header carries, with a valid CRC over the
body:
bimg_len = 16
hole_offset = 8000
hole_length = 8000 (8000 + 8000 = 16000 > BLCKSZ 8192)
bimg_info = BKPIMAGE_HAS_HOLE | BKPIMAGE_APPLY |
BKPIMAGE_COMPRESS_PGLZ (0x07)
hole_length is an on-wire field only for COMPRESSED + HAS_HOLE images, which
is why the shape is specifically a compressed image.
Expected vs. actual
-------------------
- Expected: decode rejects the record with an invalid-state error, as it
does for the zero-valued cases it already checks.
- Actual (by inspection): decode accepts it and the reconstruction
arithmetic runs with a hole larger than the page.
Mechanism, with file:line into the 18.3 source
----------------------------------------------
Field widths, src/include/access/xlogreader.h:
139: uint16 hole_offset;
140: uint16 hole_length;
141: uint16 bimg_len;
The cross-checks, xlogreader.c — the comment states exactly what is checked:
1826: /*
1827: * cross-check that hole_offset > 0, hole_length > 0 and
1828: * bimg_len < BLCKSZ if the HAS_HOLE flag is set.
1829: */
1830: if ((blk->bimg_info & BKPIMAGE_HAS_HOLE) &&
1831: (blk->hole_offset == 0 ||
1832: blk->hole_length == 0 ||
1833: blk->bimg_len == BLCKSZ))
Three non-zero-ness conditions; no relation between the two fields and
BLCKSZ.
The consumer, RestoreBlockImage:
2170: memcpy(page, ptr, bkpb->hole_offset);
2172: MemSet(page + bkpb->hole_offset, 0, bkpb->hole_length);
2173: memcpy(page + (bkpb->hole_offset + bkpb->hole_length),
2174: ptr + bkpb->hole_offset,
2175: BLCKSZ - (bkpb->hole_offset + bkpb->hole_length));
page is BLCKSZ. With hole_offset = 8000 the first memcpy alone exceeds the
page. The third length, BLCKSZ - (hole_offset + hole_length), is negative
and converts to a very large size_t.
A second path through the same missing bound: the decompressors are handed
BLCKSZ - bkpb->hole_length as their output capacity into a BLCKSZ-sized tmp:
2109: if (pglz_decompress(ptr, bkpb->bimg_len, tmp.data,
2110: BLCKSZ - bkpb->hole_length, true) <
0)
2117: LZ4_decompress_safe(ptr, tmp.data, bkpb->bimg_len,
BLCKSZ - bkpb->hole_length)
2131: ZSTD_decompress(tmp.data, BLCKSZ - bkpb->hole_length,
ptr, bkpb->bimg_len)
With hole_length > BLCKSZ that expression underflows, so a decompressor is
told it has far more room than it does.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ
@ 2026-09-04 08:01 Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-26 16:54 ` Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Rahul Yadav <rahul@rhyadav.com>
0 siblings, 1 reply; 7+ messages in thread
From: Grigorev Jurij @ 2026-09-04 08:01 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; +Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; Michael Malis <malis@pgrust.com>
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=
Attachments:
[application/octet-stream] 0001-Reject-WAL-block-images-whose-hole-does-not-fit.patch (3.3K, ../../cc0a5886d2a74f99ae58c1647ea5ed8b@localhost.localdomain/2-0001-Reject-WAL-block-images-whose-hole-does-not-fit.patch)
download | inline diff:
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Yuriy Grigoryev <ju.grigorev@ftdata.ru>
Date: Fri, 4 Sep 2026 14:20:00 +0700
Subject: [PATCH] Reject WAL block images whose hole does not fit in the page.
DecodeXLogRecord() already rejects a BKPIMAGE_HAS_HOLE image when
hole_offset or hole_length is zero, or when bimg_len is BLCKSZ. It
does not check that the hole lies inside the page. hole_offset and
hole_length are uint16 values taken from the WAL record, so a
compressed image can claim a hole that starts near the end of the
page and extends well past BLCKSZ.
RestoreBlockImage() then uses those fields as memcpy/MemSet lengths
into a BLCKSZ buffer, and as BLCKSZ - hole_length for the
decompressor's output capacity. A hole that does not fit causes
out-of-bounds writes and size_t underflow.
Extend the existing HAS_HOLE cross-check with hole_offset <= BLCKSZ
and hole_length <= BLCKSZ - hole_offset, using subtraction so the
two untrusted fields are never added together. Re-check the same
bound at the start of RestoreBlockImage() before decompression or
memcpy.
This is not reachable from WAL produced by a healthy PostgreSQL
instance. It requires a corrupt or crafted record that still has a
plausible structure.
Discussion: https://postgr.es/m/19599-8859c3822a831331@postgresql.org
---
src/backend/access/transam/xlogreader.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 5c26d33a603..b7d082d8f8b 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -1824,13 +1824,18 @@ DecodeXLogRecord(XLogReaderState *state,
datatotal += blk->bimg_len;
/*
- * cross-check that hole_offset > 0, hole_length > 0 and
- * bimg_len < BLCKSZ if the HAS_HOLE flag is set.
+ * cross-check that hole_offset > 0, hole_length > 0,
+ * bimg_len < BLCKSZ, and the hole fits in the page if the
+ * HAS_HOLE flag is set. Compare hole_length with
+ * BLCKSZ - hole_offset so the two untrusted fields are never
+ * added together.
*/
if ((blk->bimg_info & BKPIMAGE_HAS_HOLE) &&
(blk->hole_offset == 0 ||
blk->hole_length == 0 ||
- blk->bimg_len == BLCKSZ))
+ blk->bimg_len == BLCKSZ ||
+ blk->hole_offset > BLCKSZ ||
+ blk->hole_length > BLCKSZ - blk->hole_offset))
{
report_invalid_record(state,
"BKPIMAGE_HAS_HOLE set, but hole offset %u length %u block image length %u at %X/%X",
@@ -2099,6 +2104,21 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page)
bkpb = &record->record->blocks[block_id];
ptr = bkpb->bkp_image;
+ /*
+ * The hole must fit in the page. DecodeXLogRecord() already enforces
+ * this; re-check here before using the values as memcpy/MemSet lengths
+ * or as the decompressor output capacity.
+ */
+ if (bkpb->hole_offset > BLCKSZ ||
+ bkpb->hole_length > BLCKSZ - bkpb->hole_offset)
+ {
+ report_invalid_record(record,
+ "could not restore image at %X/%X with invalid state, block %d",
+ LSN_FORMAT_ARGS(record->ReadRecPtr),
+ block_id);
+ return false;
+ }
+
if (BKPIMAGE_COMPRESSED(bkpb->bimg_info))
{
/* If a backup block image is compressed, decompress it */
--
2.39.2
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ
2026-09-04 08:01 Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Grigorev Jurij <ju.grigorev@ftdata.ru>
@ 2026-09-26 16:54 ` Rahul Yadav <rahul@rhyadav.com>
2026-09-29 08:37 ` Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Grigorev Jurij <ju.grigorev@ftdata.ru>
0 siblings, 1 reply; 7+ messages in thread
From: Rahul Yadav @ 2026-09-26 16:54 UTC (permalink / raw)
To: ju.grigorev@ftdata.ru; +Cc: pgsql-hackers@lists.postgresql.org; pgsql-bugs@lists.postgresql.org; malis@pgrust.com
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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ
2026-09-04 08:01 Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-26 16:54 ` Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Rahul Yadav <rahul@rhyadav.com>
@ 2026-09-29 08:37 ` Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-30 10:07 ` Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Rahul Yadav <rahul@rhyadav.com>
2026-09-30 23:53 ` Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 7+ messages in thread
From: Grigorev Jurij @ 2026-09-29 08:37 UTC (permalink / raw)
To: Rahul Yadav <rahul@rhyadav.com>; +Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
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:
[application/octet-stream] 0001-Reject-WAL-block-images-whose-hole-does-not-fit-v2.patch (3.6K, ../../cd00a0536c674fac9e5dd9f4e27ae868@localhost.localdomain/2-0001-Reject-WAL-block-images-whose-hole-does-not-fit-v2.patch)
download | inline diff:
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Yuriy Grigoryev <ju.grigorev@ftdata.ru>
Date: Tue, 29 Sep 2026 00:00:00 +0700
Subject: [PATCH v2 1/2] Reject WAL block images whose hole does not fit
DecodeXLogRecord() cross-checked a block image with BKPIMAGE_HAS_HOLE
only for non-zero hole_offset/hole_length and bimg_len < BLCKSZ, but
never checked that the hole itself fits in the page. Both fields are
uint16s taken from the record, so e.g. hole_offset = 8000,
hole_length = 8000 (sum 16000 > BLCKSZ) with a tiny bimg_len passed
validation. RestoreBlockImage() then used them as memcpy/MemSet
lengths, with the last length (BLCKSZ - (hole_offset + hole_length))
underflowing to a huge size_t, and the decompressors were told the
output buffer was BLCKSZ - hole_length bytes.
This covers both shapes: compressed images, where hole_length is
stored in the WAL, and uncompressed images, where hole_length is
derived as BLCKSZ - bimg_len and nothing checked hole_offset against
bimg_len. For the uncompressed case the new bound amounts to
requiring hole_offset <= bimg_len.
Add the missing bound to the existing HAS_HOLE check:
hole_offset > BLCKSZ ||
hole_length > BLCKSZ - hole_offset
using subtraction so the two untrusted uint16s are never added
together. A bad hole keeps the existing HAS_HOLE error message.
DecodeXLogRecord() is the only place that fills in the image fields
of DecodedBkpBlock, so re-check the same condition with Asserts at
the start of RestoreBlockImage(), before decompression or memcpy.
XLogRecordAssemble() only creates a hole when pd_lower >=
SizeOfPageHeaderData, pd_upper > pd_lower and pd_upper <= BLCKSZ, so
no WAL written by PostgreSQL can fail the new check.
Reported-by: Michael Malis <malis@pgrust.com>
Reviewed-by: Rahul Yadav <rahul@rhyadav.com>
Discussion: https://postgr.es/m/cc0a5886d2a74f99ae58c1647ea5ed8b@localhost.localdomain
---
src/backend/access/transam/xlogreader.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -1874,13 +1874,18 @@
datatotal += blk->bimg_len;
/*
- * cross-check that hole_offset > 0, hole_length > 0 and
- * bimg_len < BLCKSZ if the HAS_HOLE flag is set.
+ * cross-check that hole_offset > 0, hole_length > 0,
+ * bimg_len < BLCKSZ, and the hole fits in the page if the
+ * HAS_HOLE flag is set. Compare hole_length with
+ * BLCKSZ - hole_offset so the two untrusted fields are never
+ * added together.
*/
if ((blk->bimg_info & BKPIMAGE_HAS_HOLE) &&
(blk->hole_offset == 0 ||
blk->hole_length == 0 ||
- blk->bimg_len == BLCKSZ))
+ blk->bimg_len == BLCKSZ ||
+ blk->hole_offset > BLCKSZ ||
+ blk->hole_length > BLCKSZ - blk->hole_offset))
{
report_invalid_record(state,
"BKPIMAGE_HAS_HOLE set, but hole offset %d length %d block image length %d at %X/%08X",
@@ -2149,6 +2154,15 @@
bkpb = &record->record->blocks[block_id];
ptr = bkpb->bkp_image;
+ /*
+ * The hole must fit in the page. DecodeXLogRecord() already enforces
+ * this for all records that reach here, so this cannot fail. Assert
+ * it before using the values as memcpy/MemSet lengths or as the
+ * decompressor output capacity.
+ */
+ Assert(bkpb->hole_offset <= BLCKSZ);
+ Assert(bkpb->hole_length <= BLCKSZ - bkpb->hole_offset);
+
if (BKPIMAGE_COMPRESSED(bkpb->bimg_info))
{
/* If a backup block image is compressed, decompress it */
--
2.43.0
[application/octet-stream] 0002-Add-test_xlogreader-for-malformed-FPI-hole-geometry.patch (11.4K, ../../cd00a0536c674fac9e5dd9f4e27ae868@localhost.localdomain/3-0002-Add-test_xlogreader-for-malformed-FPI-hole-geometry.patch)
download | inline diff:
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Yuriy Grigoryev <ju.grigorev@ftdata.ru>
Date: Tue, 29 Sep 2026 00:01:00 +0700
Subject: [PATCH v2 2/2] Add a frontend test for malformed WAL image hole
geometry.
Build in-memory WAL records with compressed and uncompressed full-page
images and feed them to DecodeXLogRecord(). The server cannot emit the
BUG #19599 shape itself, so a TAP test against a running instance would
still need a separate WAL generator.
Discussion: https://postgr.es/m/cc0a5886d2a74f99ae58c1647ea5ed8b@localhost.localdomain
---
diff --git a/src/test/modules/test_xlogreader/.gitignore b/src/test/modules/test_xlogreader/.gitignore
new file mode 100644
index 00000000000..5b90cc8b7af
--- /dev/null
+++ b/src/test/modules/test_xlogreader/.gitignore
@@ -0,0 +1,7 @@
+# Source files copied from src/backend/access/transam/
+/xlogreader.c
+
+/test_xlogreader
+
+# Generated by test suite
+/tmp_check/
diff --git a/src/test/modules/test_xlogreader/Makefile b/src/test/modules/test_xlogreader/Makefile
new file mode 100644
index 00000000000..dc80ef462f6
--- /dev/null
+++ b/src/test/modules/test_xlogreader/Makefile
@@ -0,0 +1,34 @@
+# src/test/modules/test_xlogreader/Makefile
+
+PGFILEDESC = "standalone XLogReader decoder tester"
+PGAPPICON = win32
+
+TAP_TESTS = 1
+
+OBJS = \
+ $(WIN32RES) \
+ test_xlogreader.o \
+ xlogreader.o
+
+EXTRA_CLEAN = test_xlogreader$(X) xlogreader.c
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_xlogreader
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS)
+
+all: test_xlogreader$(X)
+
+xlogreader.c: % : $(top_srcdir)/src/backend/access/transam/%
+ rm -f $@ && $(LN_S) $< .
+
+test_xlogreader$(X): $(OBJS)
+ $(CC) $(CFLAGS) $^ $(PG_LIBS_INTERNAL) $(LDFLAGS) $(LDFLAGS_EX) $(PG_LIBS) $(LIBS) -o $@
diff --git a/src/test/modules/test_xlogreader/meson.build b/src/test/modules/test_xlogreader/meson.build
new file mode 100644
index 00000000000..64bddae4a64
--- /dev/null
+++ b/src/test/modules/test_xlogreader/meson.build
@@ -0,0 +1,38 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+test_xlogreader_sources = files(
+ 'test_xlogreader.c',
+)
+test_xlogreader_sources += xlogreader_sources
+
+if host_system == 'windows'
+ test_xlogreader_sources += rc_bin_gen.process(win32ver_rc, extra_args: [
+ '--NAME', 'test_xlogreader',
+ '--FILEDESC', 'standalone XLogReader decoder tester',
+ ])
+endif
+
+test_xlogreader = executable('test_xlogreader',
+ test_xlogreader_sources,
+ dependencies: [frontend_code, lz4, zstd],
+ c_args: ['-DFRONTEND'],
+ kwargs: default_bin_args + {
+ 'install': false,
+ },
+)
+
+testprep_targets += test_xlogreader
+
+tests += {
+ 'name': 'test_xlogreader',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'tap': {
+ 'tests': [
+ 't/001_hole_geometry.pl',
+ ],
+ 'deps': [
+ test_xlogreader,
+ ],
+ },
+}
diff --git a/src/test/modules/test_xlogreader/t/001_hole_geometry.pl b/src/test/modules/test_xlogreader/t/001_hole_geometry.pl
new file mode 100644
index 00000000000..d4159cd62ec
--- /dev/null
+++ b/src/test/modules/test_xlogreader/t/001_hole_geometry.pl
@@ -0,0 +1,14 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my ($stdout, $stderr) = run_command(['test_xlogreader']);
+
+is($stderr, '', 'no error output');
+like($stdout, qr/All tests passed/, 'malformed hole geometry is rejected');
+
+done_testing();
diff --git a/src/test/modules/test_xlogreader/test_xlogreader.c b/src/test/modules/test_xlogreader/test_xlogreader.c
new file mode 100644
index 00000000000..d7a71d5545e
--- /dev/null
+++ b/src/test/modules/test_xlogreader/test_xlogreader.c
@@ -0,0 +1,250 @@
+/*-------------------------------------------------------------------------
+ *
+ * test_xlogreader.c
+ * Frontend harness for DecodeXLogRecord() hole-geometry checks
+ *
+ * Builds in-memory WAL records with full-page images and feeds them to
+ * DecodeXLogRecord(). Used to check that malformed hole geometry is
+ * rejected during decode, before RestoreBlockImage() can touch the page
+ * buffer.
+ *
+ * Copyright (c) 2026, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * src/test/modules/test_xlogreader/test_xlogreader.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+/*
+ * We have to use postgres.h not postgres_fe.h here, because there's so much
+ * backend-only stuff in the XLOG include files we need. But we need a
+ * frontend-ish environment otherwise. Hence this ugly hack.
+ */
+#define FRONTEND 1
+#include "postgres.h"
+
+#include <stdio.h>
+#include <string.h>
+
+#include "access/transam.h"
+#include "access/xlog_internal.h"
+#include "access/xlogreader.h"
+#include "access/xlogrecord.h"
+#include "common/relpath.h"
+#include "port/pg_crc32c.h"
+
+static void
+append_bytes(char **dst, const void *src, Size len)
+{
+ memcpy(*dst, src, len);
+ *dst += len;
+}
+
+/*
+ * Build a one-block WAL record containing a full-page image with a hole.
+ *
+ * If compressed is true, hole_length is stored on the wire (as in a
+ * corrupt or hand-built record). Otherwise the decoder derives hole_length as
+ * BLCKSZ - bimg_len.
+ *
+ * A valid CRC is filled in, matching ValidXLogRecord(), even though
+ * DecodeXLogRecord() itself does not verify the CRC.
+ */
+static XLogRecord *
+build_hole_image_record(char *buf, Size buflen,
+ uint16 hole_offset, uint16 hole_length,
+ uint16 bimg_len, bool compressed)
+{
+ XLogRecord *record;
+ char *p;
+ uint8 id;
+ uint8 fork_flags;
+ uint8 bimg_info;
+ uint16 data_len;
+ RelFileLocator rlocator;
+ BlockNumber blkno;
+ pg_crc32c crc;
+ char image[BLCKSZ];
+
+ if (bimg_len > BLCKSZ ||
+ buflen < SizeOfXLogRecord + MaxSizeOfXLogRecordBlockHeader + bimg_len)
+ {
+ fprintf(stderr, "internal error: cannot build WAL image record\n");
+ exit(1);
+ }
+
+ memset(buf, 0, buflen);
+ memset(image, 0xA5, sizeof(image));
+
+ record = (XLogRecord *) buf;
+ p = buf + SizeOfXLogRecord;
+
+ id = 0;
+ fork_flags = MAIN_FORKNUM | BKPBLOCK_HAS_IMAGE;
+ data_len = 0;
+ append_bytes(&p, &id, sizeof(id));
+ append_bytes(&p, &fork_flags, sizeof(fork_flags));
+ append_bytes(&p, &data_len, sizeof(data_len));
+
+ bimg_info = BKPIMAGE_HAS_HOLE | BKPIMAGE_APPLY;
+ if (compressed)
+ bimg_info |= BKPIMAGE_COMPRESS_PGLZ;
+
+ append_bytes(&p, &bimg_len, sizeof(bimg_len));
+ append_bytes(&p, &hole_offset, sizeof(hole_offset));
+ append_bytes(&p, &bimg_info, sizeof(bimg_info));
+ if (compressed)
+ append_bytes(&p, &hole_length, sizeof(hole_length));
+
+ rlocator.spcOid = 1663;
+ rlocator.dbOid = 5;
+ rlocator.relNumber = 16384;
+ blkno = 0;
+ append_bytes(&p, &rlocator, sizeof(rlocator));
+ append_bytes(&p, &blkno, sizeof(blkno));
+ append_bytes(&p, image, bimg_len);
+
+ record->xl_tot_len = p - buf;
+ record->xl_xid = InvalidTransactionId;
+ record->xl_prev = InvalidXLogRecPtr;
+ record->xl_info = 0;
+ record->xl_rmid = RM_XLOG_ID;
+
+ INIT_CRC32C(crc);
+ COMP_CRC32C(crc, buf + SizeOfXLogRecord,
+ record->xl_tot_len - SizeOfXLogRecord);
+ COMP_CRC32C(crc, buf, offsetof(XLogRecord, xl_crc));
+ FIN_CRC32C(crc);
+ record->xl_crc = crc;
+
+ return record;
+}
+
+static bool
+try_decode(XLogReaderState *state, XLogRecord *record, char **errormsg)
+{
+ DecodedXLogRecord *decoded;
+ bool ok;
+
+ decoded = (DecodedXLogRecord *)
+ palloc(DecodeXLogRecordRequiredSpace(record->xl_tot_len));
+ decoded->oversized = true;
+ state->ReadRecPtr = 0x28;
+
+ ok = DecodeXLogRecord(state, decoded, record, state->ReadRecPtr, errormsg);
+ pfree(decoded);
+ return ok;
+}
+
+static int
+run_case(XLogReaderState *state, const char *name,
+ uint16 hole_offset, uint16 hole_length, uint16 bimg_len,
+ bool compressed, bool expect_ok)
+{
+ char buf[BLCKSZ + 512];
+ XLogRecord *record;
+ char *errormsg = NULL;
+ bool ok;
+
+ record = build_hole_image_record(buf, sizeof(buf),
+ hole_offset, hole_length,
+ bimg_len, compressed);
+ ok = try_decode(state, record, &errormsg);
+
+ if (ok != expect_ok)
+ {
+ fprintf(stderr,
+ "FAIL: %s: hole_offset=%u hole_length=%u bimg_len=%u compressed=%d: expected %s, got %s%s%s\n",
+ name,
+ (unsigned int) hole_offset,
+ (unsigned int) hole_length,
+ (unsigned int) bimg_len,
+ (int) compressed,
+ expect_ok ? "accept" : "reject",
+ ok ? "accept" : "reject",
+ errormsg ? ": " : "",
+ errormsg ? errormsg : "");
+ return 1;
+ }
+
+ if (!expect_ok &&
+ (errormsg == NULL || strstr(errormsg, "BKPIMAGE_HAS_HOLE set") == NULL))
+ {
+ fprintf(stderr,
+ "FAIL: %s: rejected, but error did not mention hole geometry: %s\n",
+ name,
+ errormsg ? errormsg : "(null)");
+ return 1;
+ }
+
+ printf("ok %s\n", name);
+ return 0;
+}
+
+int
+main(int argc, char *argv[])
+{
+ XLogReaderState *state;
+ int failed = 0;
+ uint16 near_end;
+
+ (void) argc;
+ (void) argv;
+
+ state = XLogReaderAllocate(DEFAULT_XLOG_SEG_SIZE, NULL,
+ XL_ROUTINE(.page_read = NULL,
+ .segment_open = NULL,
+ .segment_close = NULL),
+ NULL);
+ if (state == NULL)
+ {
+ fprintf(stderr, "out of memory while allocating XLogReader\n");
+ return 1;
+ }
+
+ /* Use values near the end of the page, scaled to the build's BLCKSZ. */
+ near_end = (uint16) (BLCKSZ - 192);
+
+ /* Valid compressed hole entirely inside the page. */
+ failed += run_case(state, "valid compressed hole",
+ 100, 100, 16, true, true);
+
+ /* Hole ends exactly at BLCKSZ. */
+ failed += run_case(state, "valid compressed hole to end of page",
+ near_end, 192, 16, true, true);
+
+ /* Both fields large, sum past BLCKSZ. */
+ failed += run_case(state, "oversized compressed hole",
+ near_end, near_end, 16, true, false);
+
+ /* Just one byte past the end of the page. */
+ failed += run_case(state, "compressed hole one byte past page",
+ near_end, 193, 16, true, false);
+
+ /* hole_offset itself past the page. */
+ if (BLCKSZ < PG_UINT16_MAX)
+ failed += run_case(state, "hole_offset past page",
+ (uint16) (BLCKSZ + 1), 1, 16, true, false);
+
+ /*
+ * Uncompressed images store hole_offset on the wire and derive
+ * hole_length as BLCKSZ - bimg_len. A large hole_offset still has to
+ * fit in the remaining image bytes.
+ */
+ failed += run_case(state, "valid uncompressed hole",
+ 24, 0, 100, false, true);
+ failed += run_case(state, "uncompressed hole_offset past image",
+ near_end, 0, 16, false, false);
+
+ XLogReaderFree(state);
+
+ if (failed)
+ {
+ fprintf(stderr, "%d test(s) failed\n", failed);
+ return 1;
+ }
+
+ printf("All tests passed\n");
+ return 0;
+}
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -43,7 +43,8 @@ SUBDIRS = \
test_shm_mq \
test_slru \
test_tidstore \
test_wait_lsn \
+ test_xlogreader \
unsafe_tests \
worker_spi \
xid_wraparound
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -42,7 +42,8 @@ subdir('test_rls_hooks')
subdir('test_shm_mq')
subdir('test_slru')
subdir('test_tidstore')
subdir('test_wait_lsn')
+subdir('test_xlogreader')
subdir('typcache')
subdir('unsafe_tests')
subdir('worker_spi')
--
2.39.2
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ
2026-09-04 08:01 Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-26 16:54 ` Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Rahul Yadav <rahul@rhyadav.com>
2026-09-29 08:37 ` Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Grigorev Jurij <ju.grigorev@ftdata.ru>
@ 2026-09-30 10:07 ` Rahul Yadav <rahul@rhyadav.com>
1 sibling, 0 replies; 7+ messages in thread
From: Rahul Yadav @ 2026-09-30 10:07 UTC (permalink / raw)
To: ju.grigorev@ftdata.ru; +Cc: pgsql-hackers@lists.postgresql.org
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
#!/usr/bin/env python3
"""Corrupt the hole of one full-page image in a copy of a WAL directory.
Usage: corrupt_fpi.py <pg_wal copy> <pg_waldump> <start lsn>
Finds the first record whose block 0 carries an FPI with a hole, moves the
hole so it no longer fits in BLCKSZ, recomputes the record CRC, and prints
the record's LSN. The WAL is modified in place (use a copy).
"""
import re, struct, subprocess, sys
BLCKSZ, XLOG_BLCKSZ, SEGSZ = 8192, 8192, 16 * 1024 * 1024
SHORT_PHD, LONG_PHD, SIZEOF_XLOGRECORD = 24, 40, 24
BKPBLOCK_HAS_IMAGE, BKPIMAGE_HAS_HOLE, BKPIMAGE_COMPRESSED = 0x10, 0x01, 0x04 | 0x08 | 0x10
# CRC-32C (Castagnoli), reflected, as PostgreSQL's pg_crc32c
TABLE = []
for i in range(256):
c = i
for _ in range(8):
c = (c >> 1) ^ 0x82F63B78 if c & 1 else c >> 1
TABLE.append(c)
def crc_update(crc, data):
for b in data:
crc = TABLE[(crc ^ b) & 0xFF] ^ (crc >> 8)
return crc
def record_crc(rec):
# same order as XLogRecordAssemble/ValidXLogRecord: payload first, then header up to xl_crc
crc = crc_update(0xFFFFFFFF, rec[SIZEOF_XLOGRECORD:])
crc = crc_update(crc, rec[:20])
return crc ^ 0xFFFFFFFF
def segfile(waldir, lsn, tli=1):
segno = lsn // SEGSZ
return f"{waldir}/{tli:08X}{segno // 256:08X}{segno % 256:08X}"
def physical_offsets(lsn, length):
"""Map logical record bytes to physical offsets in the segment, skipping page headers."""
off, out = lsn % SEGSZ, []
while len(out) < length:
if off % XLOG_BLCKSZ == 0:
off += LONG_PHD if off == 0 else SHORT_PHD
out.append(off)
off += 1
if out[-1] >= SEGSZ:
raise ValueError("record crosses a segment boundary")
return out
waldir, waldump, start = sys.argv[1:4]
dump = subprocess.run([waldump, "-p", waldir, "-s", start, "-b"], capture_output=True, text=True).stdout
records = re.split(r"\n(?=rmgr:)", dump)
for r in records:
m = re.search(r"len \(rec/tot\):\s*\d+/\s*(\d+).*?lsn: ([0-9A-F]+)/([0-9A-F]+)", r)
b = re.search(r"blkref #0:.*?FPW.*?hole: offset: (\d+), length: (\d+)", r)
if not (m and b):
continue
tot, lsn = int(m.group(1)), (int(m.group(2), 16) << 32) | int(m.group(3), 16)
if lsn % XLOG_BLCKSZ > XLOG_BLCKSZ - 64: # keep the headers we edit on one page
continue
try:
offs = physical_offsets(lsn, tot)
except ValueError:
continue
path = segfile(waldir, lsn)
data = bytearray(open(path, "rb").read())
rec = bytearray(data[o] for o in offs)
stored = struct.unpack_from("<I", rec, 20)[0]
assert record_crc(rec) == stored, "CRC implementation/extraction mismatch"
block_id, fork_flags = rec[24], rec[25]
assert block_id == 0 and fork_flags & BKPBLOCK_HAS_IMAGE
bimg_len, hole_offset, bimg_info = struct.unpack_from("<HHB", rec, 28)
assert bimg_info & BKPIMAGE_HAS_HOLE
compressed = bool(bimg_info & BKPIMAGE_COMPRESSED)
hole_length = struct.unpack_from("<H", rec, 33)[0] if compressed else BLCKSZ - bimg_len
# push the hole past the end of the page, leaving everything else (incl. compressed data) intact
new_offset = BLCKSZ - hole_length + 1000
struct.pack_into("<H", rec, 30, new_offset)
struct.pack_into("<I", rec, 20, record_crc(rec))
for i in range(40): # write back the (single-page) header region
data[offs[i]] = rec[i]
open(path, "wb").write(data)
print(f"lsn={lsn >> 32:X}/{lsn & 0xFFFFFFFF:08X} compressed={compressed} bimg_len={bimg_len} "
f"hole_offset {hole_offset} -> {new_offset}, hole_length={hole_length} "
f"(offset+length={new_offset + hole_length} > BLCKSZ) crc {stored:08x} -> {struct.unpack_from('<I', rec, 20)[0]:08x}")
break
else:
sys.exit("no suitable FPI record found")
Attachments:
[text/plain] corrupt_fpi.py (3.7K, ../../CAJJjRReJ3vnwX6MdWifG9UAPcBp=1ag3cuu3i8u71QNcn3j9ag@mail.gmail.com/2-corrupt_fpi.py)
download | inline:
#!/usr/bin/env python3
"""Corrupt the hole of one full-page image in a copy of a WAL directory.
Usage: corrupt_fpi.py <pg_wal copy> <pg_waldump> <start lsn>
Finds the first record whose block 0 carries an FPI with a hole, moves the
hole so it no longer fits in BLCKSZ, recomputes the record CRC, and prints
the record's LSN. The WAL is modified in place (use a copy).
"""
import re, struct, subprocess, sys
BLCKSZ, XLOG_BLCKSZ, SEGSZ = 8192, 8192, 16 * 1024 * 1024
SHORT_PHD, LONG_PHD, SIZEOF_XLOGRECORD = 24, 40, 24
BKPBLOCK_HAS_IMAGE, BKPIMAGE_HAS_HOLE, BKPIMAGE_COMPRESSED = 0x10, 0x01, 0x04 | 0x08 | 0x10
# CRC-32C (Castagnoli), reflected, as PostgreSQL's pg_crc32c
TABLE = []
for i in range(256):
c = i
for _ in range(8):
c = (c >> 1) ^ 0x82F63B78 if c & 1 else c >> 1
TABLE.append(c)
def crc_update(crc, data):
for b in data:
crc = TABLE[(crc ^ b) & 0xFF] ^ (crc >> 8)
return crc
def record_crc(rec):
# same order as XLogRecordAssemble/ValidXLogRecord: payload first, then header up to xl_crc
crc = crc_update(0xFFFFFFFF, rec[SIZEOF_XLOGRECORD:])
crc = crc_update(crc, rec[:20])
return crc ^ 0xFFFFFFFF
def segfile(waldir, lsn, tli=1):
segno = lsn // SEGSZ
return f"{waldir}/{tli:08X}{segno // 256:08X}{segno % 256:08X}"
def physical_offsets(lsn, length):
"""Map logical record bytes to physical offsets in the segment, skipping page headers."""
off, out = lsn % SEGSZ, []
while len(out) < length:
if off % XLOG_BLCKSZ == 0:
off += LONG_PHD if off == 0 else SHORT_PHD
out.append(off)
off += 1
if out[-1] >= SEGSZ:
raise ValueError("record crosses a segment boundary")
return out
waldir, waldump, start = sys.argv[1:4]
dump = subprocess.run([waldump, "-p", waldir, "-s", start, "-b"], capture_output=True, text=True).stdout
records = re.split(r"\n(?=rmgr:)", dump)
for r in records:
m = re.search(r"len \(rec/tot\):\s*\d+/\s*(\d+).*?lsn: ([0-9A-F]+)/([0-9A-F]+)", r)
b = re.search(r"blkref #0:.*?FPW.*?hole: offset: (\d+), length: (\d+)", r)
if not (m and b):
continue
tot, lsn = int(m.group(1)), (int(m.group(2), 16) << 32) | int(m.group(3), 16)
if lsn % XLOG_BLCKSZ > XLOG_BLCKSZ - 64: # keep the headers we edit on one page
continue
try:
offs = physical_offsets(lsn, tot)
except ValueError:
continue
path = segfile(waldir, lsn)
data = bytearray(open(path, "rb").read())
rec = bytearray(data[o] for o in offs)
stored = struct.unpack_from("<I", rec, 20)[0]
assert record_crc(rec) == stored, "CRC implementation/extraction mismatch"
block_id, fork_flags = rec[24], rec[25]
assert block_id == 0 and fork_flags & BKPBLOCK_HAS_IMAGE
bimg_len, hole_offset, bimg_info = struct.unpack_from("<HHB", rec, 28)
assert bimg_info & BKPIMAGE_HAS_HOLE
compressed = bool(bimg_info & BKPIMAGE_COMPRESSED)
hole_length = struct.unpack_from("<H", rec, 33)[0] if compressed else BLCKSZ - bimg_len
# push the hole past the end of the page, leaving everything else (incl. compressed data) intact
new_offset = BLCKSZ - hole_length + 1000
struct.pack_into("<H", rec, 30, new_offset)
struct.pack_into("<I", rec, 20, record_crc(rec))
for i in range(40): # write back the (single-page) header region
data[offs[i]] = rec[i]
open(path, "wb").write(data)
print(f"lsn={lsn >> 32:X}/{lsn & 0xFFFFFFFF:08X} compressed={compressed} bimg_len={bimg_len} "
f"hole_offset {hole_offset} -> {new_offset}, hole_length={hole_length} "
f"(offset+length={new_offset + hole_length} > BLCKSZ) crc {stored:08x} -> {struct.unpack_from('<I', rec, 20)[0]:08x}")
break
else:
sys.exit("no suitable FPI record found")
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ
2026-09-04 08:01 Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-26 16:54 ` Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Rahul Yadav <rahul@rhyadav.com>
2026-09-29 08:37 ` Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Grigorev Jurij <ju.grigorev@ftdata.ru>
@ 2026-09-30 23:53 ` Michael Paquier <michael@paquier.xyz>
2026-10-01 03:18 ` Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Grigorev Jurij <ju.grigorev@ftdata.ru>
1 sibling, 1 reply; 7+ messages in thread
From: Michael Paquier @ 2026-09-30 23:53 UTC (permalink / raw)
To: Grigorev Jurij <ju.grigorev@ftdata.ru>; +Cc: Rahul Yadav <rahul@rhyadav.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ar2hA_b1Be7VsZdP@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ
2026-09-04 08:01 Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-26 16:54 ` Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Rahul Yadav <rahul@rhyadav.com>
2026-09-29 08:37 ` Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-30 23:53 ` Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Michael Paquier <michael@paquier.xyz>
@ 2026-10-01 03:18 ` Grigorev Jurij <ju.grigorev@ftdata.ru>
0 siblings, 0 replies; 7+ messages in thread
From: Grigorev Jurij @ 2026-10-01 03:18 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Rahul Yadav <rahul@rhyadav.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
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
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-10-01 03:18 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-08-02 17:51 BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ PG Bug reporting form <noreply@postgresql.org>
2026-09-04 08:01 Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-26 16:54 ` Rahul Yadav <rahul@rhyadav.com>
2026-09-29 08:37 ` Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-30 10:07 ` Rahul Yadav <rahul@rhyadav.com>
2026-09-30 23:53 ` Michael Paquier <michael@paquier.xyz>
2026-10-01 03:18 ` Grigorev Jurij <ju.grigorev@ftdata.ru>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox