Hi Yuriy, I reviewed and tested v1. The fix looks right to me, and I think it should go in. Some comments below.
Testing (macOS arm64, Apple clang 17, meson debug build with assertions, master at f25c50fd8f plus v1 as cfbot applies it): - The regression tests and the pg_walinspect tests pass. - Crash recovery with wal_compression = pglz, lz4, zstd and off, each replaying about 1,100 full-page images with holes, plus a pglz run with wal_consistency_checking = all (about 21,000 images). The data matched before and after the crash, and amcheck (verify_heapam, and bt_index_check with heapallindexed) found nothing. - I crafted records whose hole doesn't fit in the page (hole_offset moved so that hole_offset + hole_length = 9192, record CRC recomputed), once in an uncompressed FPI and once in a pglz-compressed one. Without the patch, pg_waldump decodes both records without complaint, and pg_waldump --save-fullpage crashes on both (SIGBUS and SIGSEGV). With the patch, both are rejected at decode time, and crash recovery over the same WAL stops there with BKPIMAGE_HAS_HOLE set, but hole offset 2364 length 6828 block image length 416 at ... instead of writing past the page. Comments: 1. Uncompressed images are affected too, not only compressed ones. There, hole_length is derived as BLCKSZ - bimg_len, and nothing checked hole_offset against bimg_len. The new condition covers that case as well (for an uncompressed image it amounts to requiring hole_offset <= bimg_len), so the commit message could say so; right now it only mentions compressed images. 2. v1 doesn't apply to master with git am. The context line in DecodeXLogRecord() still reads "hole offset %u ... at %X/%X", so I guess it was made against an older branch. cfbot's copy applies, but a rebased v2 would make it easier to test. 3. The new message in RestoreBlockImage() uses %X/%X, while everything else in xlogreader.c uses %X/%08X now. Its text is also identical to the existing message for a block without an image, so the two failures can't be told apart in the log. 4. DecodeXLogRecord() is the only place that fills in the image fields of DecodedBkpBlock, so with this patch the re-check in RestoreBlockImage() can't fail; in my tests it never did. I'd make it an Assert(). If you prefer a runtime check, a distinct message would help (see 3). About back-patching: XLogRecordAssemble() only creates a hole when pd_lower >= SizeOfPageHeaderData, pd_upper > pd_lower and pd_upper <= BLCKSZ, so hole_offset + hole_length <= BLCKSZ for any WAL that PostgreSQL writes. The new check can't reject valid WAL, and it turns memory corruption on a bad record into a clean error. So +1 for back-patching from me. Unrelated to this patch, but noticed while testing: the error messages in DecodeXLogRecord() print state->ReadRecPtr rather than the lsn of the record being decoded. With read-ahead that's an earlier record: pg_waldump reported the corrupt record at 0/0197B3A0 (the previous record) instead of 0/0197B7E8, and in recovery the reported LSN was two records back. I can send a separate patch for that. It would still be good to see the frontend test you mentioned. I can also share the script I used to craft the records if that helps. Regards, Rahul Yadav
