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)
