- 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, 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:
> - /* 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.
