Avoid somewhat-theoretical overflow risks in RecordIsValid().

This improves on commit 51fed14d73ed3acd2282b531fb1396877e44e86a by
eliminating the assumption that we can form <some pointer value> +
<some offset> without overflow.  The entire point of those tests is that
we don't trust the offset value, so coding them in a way that could wrap
around if the buffer happens to be near the top of memory doesn't seem
sound.  Instead, track the remaining space as a size_t variable and
compare offsets against that.

Also, improve comment about why we need the extra early check on
xl_tot_len.

1 files changed, 17 insertions, 9 deletions

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f24e2fb63e9..a7762eafcd0 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3697,7 +3697,10 @@ RestoreBkpBlocks(XLogRecPtr lsn, XLogRecord *record, bool cleanup)
  * record (other than to the minimal extent of computing the amount of
  * data to read in) until we've checked the CRCs.
  *
- * We assume all of the record has been read into memory at *record.
+ * We assume all of the record (that is, xl_tot_len bytes) has been read
+ * into memory at *record.  Also, ValidXLogRecordHeader() has accepted the
+ * record's header, which means in particular that xl_tot_len is at least
+ * SizeOfXlogRecord, so it is safe to fetch xl_len.
  */
 static bool
 RecordIsValid(XLogRecord *record, XLogRecPtr recptr, int emode)
@@ -3707,10 +3710,10 @@ RecordIsValid(XLogRecord *record, XLogRecPtr recptr, int emode)
 	uint32		len = record->xl_len;
 	BkpBlock	bkpb;
 	char	   *blk;
-	char	   *recend = (char *) record + record->xl_tot_len;
+	size_t		remaining = record->xl_tot_len;
 
 	/* First the rmgr data */
-	if (XLogRecGetData(record) + len > recend)
+	if (remaining < SizeOfXLogRecord + len)
 	{
 		/* ValidXLogRecordHeader() should've caught this already... */
 		ereport(emode_for_corrupt_record(emode, recptr),
@@ -3718,6 +3721,7 @@ RecordIsValid(XLogRecord *record, XLogRecPtr recptr, int emode)
 						(uint32) (recptr >> 32), (uint32) recptr)));
 		return false;
 	}
+	remaining -= SizeOfXLogRecord + len;
 	INIT_CRC32(crc);
 	COMP_CRC32(crc, XLogRecGetData(record), len);
 
@@ -3730,10 +3734,10 @@ RecordIsValid(XLogRecord *record, XLogRecPtr recptr, int emode)
 		if (!(record->xl_info & XLR_SET_BKP_BLOCK(i)))
 			continue;
 
-		if (blk + sizeof(BkpBlock) > recend)
+		if (remaining < sizeof(BkpBlock))
 		{
 			ereport(emode_for_corrupt_record(emode, recptr),
-					(errmsg("incorrect backup block size in record at %X/%X",
+					(errmsg("invalid backup block size in record at %X/%X",
 							(uint32) (recptr >> 32), (uint32) recptr)));
 			return false;
 		}
@@ -3748,19 +3752,20 @@ RecordIsValid(XLogRecord *record, XLogRecPtr recptr, int emode)
 		}
 		blen = sizeof(BkpBlock) + BLCKSZ - bkpb.hole_length;
 
-		if (blk + blen > recend)
+		if (remaining < blen)
 		{
 			ereport(emode_for_corrupt_record(emode, recptr),
 					(errmsg("invalid backup block size in record at %X/%X",
 							(uint32) (recptr >> 32), (uint32) recptr)));
 			return false;
 		}
+		remaining -= blen;
 		COMP_CRC32(crc, blk, blen);
 		blk += blen;
 	}
 
 	/* Check that xl_tot_len agrees with our calculation */
-	if (blk != (char *) record + record->xl_tot_len)
+	if (remaining != 0)
 	{
 		ereport(emode_for_corrupt_record(emode, recptr),
 				(errmsg("incorrect total length in record at %X/%X",
@@ -3904,8 +3909,11 @@ retry:
 
 	/*
 	 * If the whole record header is on this page, validate it immediately.
-	 * Otherwise only do a basic sanity check on xl_tot_len, and validate the
-	 * rest of the header after reading it from the next page.
+	 * Otherwise do just a basic sanity check on xl_tot_len, and validate the
+	 * rest of the header after reading it from the next page.  The xl_tot_len
+	 * check is necessary here to ensure that we enter the "Need to reassemble
+	 * record" code path below; otherwise we might fail to apply
+	 * ValidXLogRecordHeader at all.
 	 */
 	if (targetRecOff <= XLOG_BLCKSZ - SizeOfXLogRecord)
 	{