Re: BUG #19599: RestoreBlockImage: the decode cross-checks never bound hole_offset + hole_length against BLCKSZ

Hi,

Michael reported this as BUG #19599. He wasn't sure it was a real issue, but
I think the missing check is still worth fixing. Postgres never writes a
hole that doesn't fit in the page, but DecodeXLogRecord() will happily
accept one from a corrupt or hand-built record, and RestoreBlockImage()
then uses those fields as memcpy/MemSet lengths.

What decode checks today is only non-zero values:

> if ((blk->bimg_info & BKPIMAGE_HAS_HOLE) &&
> (blk->hole_offset == 0 ||
> blk->hole_length == 0 ||
> blk->bimg_len == BLCKSZ))

It never asks whether the hole actually fits in the page. Both fields
are uint16s taken from the record, so this gets through:

hole_offset = 8000
hole_length = 8000 /* 16000 > BLCKSZ */
bimg_len = 16
bimg_info = HAS_HOLE | APPLY | COMPRESS_PGLZ

That's specifically a compressed image, because that's the only shape
where hole_length is stored in the WAL rather than computed. The CRC
can still be valid. After that, RestoreBlockImage() does:

memcpy(page, ptr, hole_offset);
MemSet(page + hole_offset, 0, hole_length);
memcpy(..., BLCKSZ - (hole_offset + hole_length));

With the numbers above, the first memcpy already runs off the end of an
8kB page, and the last length underflows to a huge size_t. The
decompressors have the same problem: they are told the output buffer is
BLCKSZ - hole_length bytes.

I've attached the patch that adds the missing bound to that existing HAS_HOLE
check:

hole_offset > BLCKSZ ||
hole_length > BLCKSZ - hole_offset

(subtraction rather than addition, so the two uint16s can't overflow.)
The same check is repeated at the start of RestoreBlockImage(), before
decompression or memcpy. A bad hole still uses the existing HAS_HOLE
error message, I didn't add a new one.

I also have a small frontend test that builds this record in memory (valid
header and CRC, compressed image, bad hole) and feeds it to
DecodeXLogRecord(). Happy to send that if it's useful, I left it out of
this mail so the patch stays small.

Does this look like the right approach?
I'd also like to hear whether it's worth back-patching.

Thanks,
Yuriy Grigoryev