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

On Fri, Jul 31, 2026 at 2:34 AM Matthias van de Meent <
boekewurm+postgres(at)gmail(dot)com> wrote:

> On Fri, 31 Jul 2026 at 10:25, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >
> > On Thu, Jul 30, 2026 at 01:36:00PM -0700, David K wrote:
> > > Thanks Matthias for the review.
> >
> > Please do not top-post, see this link, in line with the Postgres
> > mailing list style:
>
> +1
>
> > > 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
> >
> > I doubt that we want a full-fledged test module for this purpose
> > integrated in the end result, even if it proves your point. The
> > overall value does not seem to pile up from my perspective, just for a
> > bunch of edge-case artistic checks to validate the API.
>
> I agree that testing just these changes doesn't add much value, and
> definitely not in the way currently implemented, but I do think there
> would be value in a module that tests the various kinds of corruption
> the XLogReader could encounter and should detect.
>
> > + /*
> > + * Cap xl_tot_len before contrecord reassembly so we never
> allocate or
> > + * copy based on a garbage length from a recycled page.
> > + */
> > + if (total_len > XLogRecordMaxSize)
> > + {
> > + report_invalid_record(state,
> > + "invalid record length at %X/%08X:
> expected at most %u, got %u",
> > + LSN_FORMAT_ARGS(RecPtr),
> > + XLogRecordMaxSize, total_len);
> > + goto err;
> > + }
> >
> > Saying that. Andres has reminded me not so long ago that we enforce
> > this rule on the WAL insert side but we don't do so on the xlogreader
> > side. Now, what's the point in having the same check two times? The
> > proposed patch does it once we have read the first bytes of the record
> > for the total record length, and a second time when validating the
> > record header. It feels to me that we should just do it once. Or,
> > wait, you have done it this way to map with total_len? In which case
> > I can buy it.. It seems to me that this should be split as a patch of
> > its own.
>
> Note that the record header can be split across pages, and allocations
> currently happen ahead of the validation of that split header. I think
> the additional check makes sense here.
>
> > On Thu, Jul 30, 2026 at 01:36:00PM -0700, David K wrote:
> > > 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
>
> I'd personally hoped the adjusted patch would've been more like
> attached: In it, I've cleaned up allocate_recordbuf to avoid the
> additional XLOG_BLCKSZ and adjusted a few comments. It also drops the
> test module.
>
> Kind regards,
>
> Matthias van de Meent
>

Thanks. That seems fine to me so is there anything else I need to do on my
end?