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

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:
https://en.wikipedia.org/wiki/Posting_style#Bottom-posting

Yep, we are old-school around here. :p

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

+ /*
+ * 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.
--
Michael