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

On Mon, 27 Jul 2026 at 12:20, David K <dkarapetyan(at)gmail(dot)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)