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 < [email protected]> wrote: > On Mon, 27 Jul 2026 at 12:20, David K <[email protected]> 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) >
0001-v2-Fix-XLogReader-mishandling-of-oversized-multi-page-records.patch
Description: Binary data
