From 10685ec082181b285a48d982b4c3463fd80ee5ae Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 21 Aug 2012 18:41:52 -0400 Subject: [PATCH] Avoid somewhat-theoretical overflow risks in RecordIsValid(). This improves on commit 51fed14d73ed3acd2282b531fb1396877e44e86a by eliminating the assumption that we can form + 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. --- src/backend/access/transam/xlog.c | 26 +++++++++++++++++--------- 1 file 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) { -- 2.39.5