agora inbox for pgsql-bugs@postgresql.org
help / color / mirror / Atom feedFrom: Grigorev Jurij <ju.grigorev@ftdata.ru>
To: Rahul Yadav <rahul@rhyadav.com>
Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Subject: Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ
Date: Tue, 29 Sep 2026 08:37:02 +0000
Message-ID: <cd00a0536c674fac9e5dd9f4e27ae868@localhost.localdomain> (raw)
In-Reply-To: <CAJJjRReEjgkbm_J9avW=z+eqz6tjyjJRfBmyQvfSx04td-OEvQ@mail.gmail.com>
References: <cc0a5886d2a74f99ae58c1647ea5ed8b@localhost.localdomain>
<CAJJjRReEjgkbm_J9avW=z+eqz6tjyjJRfBmyQvfSx04td-OEvQ@mail.gmail.com>
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
view thread (7+ messages) latest in thread
Message-ID: <cd00a0536c674fac9e5dd9f4e27ae868@localhost.localdomain>
Permalink: ../cd00a0536c674fac9e5dd9f4e27ae868@localhost.localdomain/
Also on: postgresql.org/message-id/cd00a0536c674fac9e5dd9f4e27ae868@localhost.localdomain
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: ju.grigorev@ftdata.ru, rahul@rhyadav.com, pgsql-hackers@lists.postgresql.org
Subject: Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ
In-Reply-To: <cd00a0536c674fac9e5dd9f4e27ae868@localhost.localdomain>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox