Bug: XLogReader mishandles oversized multi-page xl_tot_len (potential memory corruption)

On Mon, 27 Jul 2026 at 12:20, David K <dkarapetyan@gmail.com> wrote:

Hi,

An automated AI review of the WAL reader found that XLogReader does not enforce XLogRecordMaxSize on xl_tot_len. The insert path has a check:
XLogRecordAssemble() rejects total_len > XLogRecordMaxSize
but the reader only checks a minimum length. That asymmetry allows a crafted or corrupted multi-page record reassembly to overflow.

Yep.

Fix
---
1. Reject xl_tot_len > XLogRecordMaxSize in ValidXLogRecordHeader(), and on the partial-header path before multi-page reassembly starts (symmetric with XLogRecordAssemble()).
2. Compute reassembly buffer sizes with size_t in allocate_recordbuf() so near-UINT32_MAX lengths cannot wrap even if a caller forgets the bound.

This is not exactly corect. The distinction between size_t and uint32
is nothing more than cosmetic on 32-bit systems, so just changing
between the types won't change a thing there. You'll have to use the
add/mul_size helpers (palloc.h) if you want to be certain unintended
overflows are detected across all platforms.

---

patch:
I only reviewed the xlogreader changes:

+++ b/src/backend/access/transam/xlogreader.c

* Note: This routine should *never* be called for xl_tot_len until the header
- * of the record has been fully validated.
+ * of the record has been fully validated (including the XLogRecordMaxSize
+ * bound).  Size math uses size_t so near-UINT32_MAX lengths cannot wrap to a
+ * small allocation.

The reclength parameter should have a value that cannot overflow with
the calculations we're doing here; that's what the new checks of the
patch prevent. An Assert() to this effect should be sufficient; the
change to size_t is therefore not necessary.

Additionally, we can avoid the additional XLOG_BLCKSZ bytes of memory
usage when the record size is a multiple of XLOG_BLCKSZ by using
correctly type-aligned lengths, like so:

-    newSize += XLOG_BLCKSZ - (newSize % XLOG_BLCKSZ);
+    newSize = TYPEALIGN(XLOG_BLCKSZ, newSize);

-        /* There may be no next page if it's too small. */
+        /*
+         * There may be no next page if it's too small.  Cap xl_tot_len before
+         * contrecord reassembly so we never allocate or copy based on a
+         * garbage length from a recycled page.
+         */

Please put the new comment content on the newly added if-statement
that actually does the record-is-oversized check.

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

Thanks Matthias for the review.

Patch is attached with the following changes:
- keep XLogRecordMaxSize checks + Assert on reclength
- use add_size/mul_size instead of a bare size_t cast
- TYPEALIGN-equivalent roundup without an extra page when already aligned
- comment on the new oversized-length check

On Mon, Jul 27, 2026 at 4:35 AM Matthias van de Meent <
boekewurm+postgres@gmail.com> wrote:

On Thu, Jul 30, 2026 at 01:36:00PM -0700, David K wrote:

Thanks Matthias for the review.

Please do not top-post, see this link, in line with the Postgres
mailing list style:
https://en.wikipedia.org/wiki/Posting_style#Bottom-posting

Yep, we are old-school around here. :p

Patch is attached with the following changes:
- keep XLogRecordMaxSize checks + Assert on reclength
- use add_size/mul_size instead of a bare size_t cast
- TYPEALIGN-equivalent roundup without an extra page when already aligned
- comment on the new oversized-length check

I doubt that we want a full-fledged test module for this purpose
integrated in the end result, even if it proves your point. The
overall value does not seem to pile up from my perspective, just for a
bunch of edge-case artistic checks to validate the API.

+        /*
+         * Cap xl_tot_len before contrecord reassembly so we never allocate or
+         * copy based on a garbage length from a recycled page.
+         */
+        if (total_len > XLogRecordMaxSize)
+        {
+            report_invalid_record(state,
+                                  "invalid record length at %X/%08X: expected at most %u, got %u",
+                                  LSN_FORMAT_ARGS(RecPtr),
+                                  XLogRecordMaxSize, total_len);
+            goto err;
+        }

Saying that. Andres has reminded me not so long ago that we enforce
this rule on the WAL insert side but we don't do so on the xlogreader
side. Now, what's the point in having the same check two times? The
proposed patch does it once we have read the first bytes of the record
for the total record length, and a second time when validating the
record header. It feels to me that we should just do it once. Or,
wait, you have done it this way to map with total_len? In which case
I can buy it.. It seems to me that this should be split as a patch of
its own.
--
Michael

On Fri, 31 Jul 2026 at 10:25, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Jul 30, 2026 at 01:36:00PM -0700, David K wrote:

Thanks Matthias for the review.

Please do not top-post, see this link, in line with the Postgres
mailing list style:

+1

Patch is attached with the following changes:
- keep XLogRecordMaxSize checks + Assert on reclength
- use add_size/mul_size instead of a bare size_t cast
- TYPEALIGN-equivalent roundup without an extra page when already aligned
- comment on the new oversized-length check

I doubt that we want a full-fledged test module for this purpose
integrated in the end result, even if it proves your point. The
overall value does not seem to pile up from my perspective, just for a
bunch of edge-case artistic checks to validate the API.

I agree that testing just these changes doesn't add much value, and
definitely not in the way currently implemented, but I do think there
would be value in a module that tests the various kinds of corruption
the XLogReader could encounter and should detect.

+        /*
+         * Cap xl_tot_len before contrecord reassembly so we never allocate or
+         * copy based on a garbage length from a recycled page.
+         */
+        if (total_len > XLogRecordMaxSize)
+        {
+            report_invalid_record(state,
+                                  "invalid record length at %X/%08X: expected at most %u, got %u",
+                                  LSN_FORMAT_ARGS(RecPtr),
+                                  XLogRecordMaxSize, total_len);
+            goto err;
+        }

Saying that. Andres has reminded me not so long ago that we enforce
this rule on the WAL insert side but we don't do so on the xlogreader
side. Now, what's the point in having the same check two times? The
proposed patch does it once we have read the first bytes of the record
for the total record length, and a second time when validating the
record header. It feels to me that we should just do it once. Or,
wait, you have done it this way to map with total_len? In which case
I can buy it.. It seems to me that this should be split as a patch of
its own.

Note that the record header can be split across pages, and allocations
currently happen ahead of the validation of that split header. I think
the additional check makes sense here.

On Thu, Jul 30, 2026 at 01:36:00PM -0700, David K wrote:

Patch is attached with the following changes:
- keep XLogRecordMaxSize checks + Assert on reclength
- use add_size/mul_size instead of a bare size_t cast
- TYPEALIGN-equivalent roundup without an extra page when already aligned
- comment on the new oversized-length check

I'd personally hoped the adjusted patch would've been more like
attached: In it, I've cleaned up allocate_recordbuf to avoid the
additional XLOG_BLCKSZ and adjusted a few comments. It also drops the
test module.

Kind regards,

Matthias van de Meent

On Fri, Jul 31, 2026 at 2:34 AM Matthias van de Meent <
boekewurm+postgres@gmail.com> wrote:

On Fri, 31 Jul 2026 at 10:25, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Jul 30, 2026 at 01:36:00PM -0700, David K wrote:

Thanks Matthias for the review.

Please do not top-post, see this link, in line with the Postgres
mailing list style:

+1

Patch is attached with the following changes:
- keep XLogRecordMaxSize checks + Assert on reclength
- use add_size/mul_size instead of a bare size_t cast
- TYPEALIGN-equivalent roundup without an extra page when already

aligned

- comment on the new oversized-length check

I doubt that we want a full-fledged test module for this purpose
integrated in the end result, even if it proves your point. The
overall value does not seem to pile up from my perspective, just for a
bunch of edge-case artistic checks to validate the API.

I agree that testing just these changes doesn't add much value, and
definitely not in the way currently implemented, but I do think there
would be value in a module that tests the various kinds of corruption
the XLogReader could encounter and should detect.

+        /*
+         * Cap xl_tot_len before contrecord reassembly so we never

allocate or

+         * copy based on a garbage length from a recycled page.
+         */
+        if (total_len > XLogRecordMaxSize)
+        {
+            report_invalid_record(state,
+                                  "invalid record length at %X/%08X:

expected at most %u, got %u",

+                                  LSN_FORMAT_ARGS(RecPtr),
+                                  XLogRecordMaxSize, total_len);
+            goto err;
+        }

Saying that. Andres has reminded me not so long ago that we enforce
this rule on the WAL insert side but we don't do so on the xlogreader
side. Now, what's the point in having the same check two times? The
proposed patch does it once we have read the first bytes of the record
for the total record length, and a second time when validating the
record header. It feels to me that we should just do it once. Or,
wait, you have done it this way to map with total_len? In which case
I can buy it.. It seems to me that this should be split as a patch of
its own.

Note that the record header can be split across pages, and allocations
currently happen ahead of the validation of that split header. I think
the additional check makes sense here.

On Thu, Jul 30, 2026 at 01:36:00PM -0700, David K wrote:

Patch is attached with the following changes:
- keep XLogRecordMaxSize checks + Assert on reclength
- use add_size/mul_size instead of a bare size_t cast
- TYPEALIGN-equivalent roundup without an extra page when already

aligned

- comment on the new oversized-length check

I'd personally hoped the adjusted patch would've been more like
attached: In it, I've cleaned up allocate_recordbuf to avoid the
additional XLOG_BLCKSZ and adjusted a few comments. It also drops the
test module.

Kind regards,

Matthias van de Meent

Thanks. That seems fine to me so is there anything else I need to do on my
end?

On Fri, Jul 31, 2026 at 11:15:08AM -0700, David K wrote:

Thanks. That seems fine to me so is there anything else I need to do on my
end?

-       newSize += XLOG_BLCKSZ - (newSize % XLOG_BLCKSZ);
+       Assert(reclength <= INT32_MAX - BLCKSZ);
+
+       newSize = TYPEALIGN(XLOG_BLCKSZ, reclength);

Am I reading a typo here or the INT32_MAX is missing a 'PG_U'?
XLogRecordMaxSize cannot reach that, just wondering about a
consistency argument with the surrounding type declarations for these
length variables.
--
Michael

On Sun, Aug 02, 2026 at 04:08:17PM +0900, Michael Paquier wrote:

-       newSize += XLOG_BLCKSZ - (newSize % XLOG_BLCKSZ);
+       Assert(reclength <= INT32_MAX - BLCKSZ);
+
+       newSize = TYPEALIGN(XLOG_BLCKSZ, reclength);

Am I reading a typo here or the INT32_MAX is missing a 'PG_U'?
XLogRecordMaxSize cannot reach that, just wondering about a
consistency argument with the surrounding type declarations for these
length variables.

Another thing to note: this basically breaks the recovery test
039_end_of_wal.pl. Could you look at that please?
--
Michael

On Mon, 3 Aug 2026 at 01:21, Michael Paquier <michael@paquier.xyz> wrote:

On Sun, Aug 02, 2026 at 04:08:17PM +0900, Michael Paquier wrote:

-       newSize += XLOG_BLCKSZ - (newSize % XLOG_BLCKSZ);
+       Assert(reclength <= INT32_MAX - BLCKSZ);
+
+       newSize = TYPEALIGN(XLOG_BLCKSZ, reclength);

Am I reading a typo here or the INT32_MAX is missing a 'PG_U'?
XLogRecordMaxSize cannot reach that, just wondering about a
consistency argument with the surrounding type declarations for these
length variables.

Yes, that was an oversight in my submission.
It's been changed to XLogRecordMaxSize, with a new comment, in the
attached patch.

Another thing to note: this basically breaks the recovery test
039_end_of_wal.pl. Could you look at that please?

Also fixed, including a new test case for >XLogRecordMaxSize.

-Matthias