Hi Michael,
Thanks for looking! I agree XLogRecordAssemble() never writes
such a hole, CRC catches accidental corruption, and core WAL is
trusted.
My only point was that DecodeXLogRecord() already distrusts these
fields enough to cross-check them: it rejects hole_offset == 0,
hole_length == 0, bimg_len == BLCKSZ when HAS_HOLE is set, and
non-zero hole fields when it is not set. Bounding
hole_offset + hole_length against BLCKSZ just completes that
existing family of checks. Rahul's repro shows a re-CRCed record
still passes decode and then crashes pg_waldump --save-fullpage.
I agree the test in 0002 is quite large for such a small check --
happy to drop it entirely. To keep this minimal, we could keep
just the two-line check in DecodeXLogRecord() with no extra test --
the existing HAS_HOLE error message, no new paths.
I don't insist on the test or backpatch -- if you prefer, let's
keep only the decode check, or close it if you think even that
is not wanted. Should xlogreader be robust here, or may
RestoreBlockImage() assume trusted input after CRC?
I understand from your message that you lean towards this not
being needed, given trusted WAL, CRC and the insertion bounds --
just wanted to understand where the line is. Happy to update
or close as you suggest.
Kind regards,
Yuriy
