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
3+ messages / 3 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; 3+ 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] 3+ 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; 3+ 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] 3+ 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>
0 siblings, 0 replies; 3+ 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] 3+ messages in thread
end of thread, other threads:[~2026-09-26 16:54 UTC | newest]
Thread overview: 3+ 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>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox