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

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(at)gmail(dot)com> wrote:

> 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)
>