database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-06-14 18:42:34 +03:00
Code Activity

Avoid somewhat-theoretical overflow risks in RecordIsValid().

Browse Source 
This improves on commit 51fed14d73 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.
This commit is contained in:
Tom Lane
2012-08-21 18:41:52 -04:00
parent 0f524ea0cf
commit 10685ec082
1 changed files with 17 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
src/backend/access/transam/xlog.c
View File

@ -3697,7 +3697,10 @@ RestoreBkpBlocks(XLogRecPtr lsn, XLogRecord *record, bool cleanup)
* record (other than to the minimal extent of computing the amount of * record (other than to the minimal extent of computing the amount of
* data to read in) until we've checked the CRCs. * 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 static bool
RecordIsValid(XLogRecord *record, XLogRecPtr recptr, int emode) RecordIsValid(XLogRecord *record, XLogRecPtr recptr, int emode)
@ -3707,10 +3710,10 @@ RecordIsValid(XLogRecord *record, XLogRecPtr recptr, int emode)
uint32 len = record->xl_len; uint32 len = record->xl_len;
BkpBlock bkpb; BkpBlock bkpb;
char *blk; char *blk;
char *recend = (char *) record + record->xl_tot_len; size_t remaining = record->xl_tot_len;
/* First the rmgr data */ /* First the rmgr data */
if (XLogRecGetData(record) + len > recend) if (remaining < SizeOfXLogRecord + len)
{ {
/* ValidXLogRecordHeader() should've caught this already... */ /* ValidXLogRecordHeader() should've caught this already... */
ereport(emode_for_corrupt_record(emode, recptr), ereport(emode_for_corrupt_record(emode, recptr),
@ -3718,6 +3721,7 @@ RecordIsValid(XLogRecord *record, XLogRecPtr recptr, int emode)
(uint32) (recptr >> 32), (uint32) recptr))); (uint32) (recptr >> 32), (uint32) recptr)));
return false; return false;
} }
remaining -= SizeOfXLogRecord + len;
INIT_CRC32(crc); INIT_CRC32(crc);
COMP_CRC32(crc, XLogRecGetData(record), len); 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))) if (!(record->xl_info & XLR_SET_BKP_BLOCK(i)))
continue; continue;
if (blk + sizeof(BkpBlock) > recend) if (remaining < sizeof(BkpBlock))
{ {
ereport(emode_for_corrupt_record(emode, recptr), 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))); (uint32) (recptr >> 32), (uint32) recptr)));
return false; return false;
} }
@ -3748,19 +3752,20 @@ RecordIsValid(XLogRecord *record, XLogRecPtr recptr, int emode)
} }
blen = sizeof(BkpBlock) + BLCKSZ - bkpb.hole_length; blen = sizeof(BkpBlock) + BLCKSZ - bkpb.hole_length;
if (blk + blen > recend) if (remaining < blen)
{ {
ereport(emode_for_corrupt_record(emode, recptr), ereport(emode_for_corrupt_record(emode, recptr),
(errmsg("invalid backup block size in record at %X/%X", (errmsg("invalid backup block size in record at %X/%X",
(uint32) (recptr >> 32), (uint32) recptr))); (uint32) (recptr >> 32), (uint32) recptr)));
return false; return false;
} }
remaining -= blen;
COMP_CRC32(crc, blk, blen); COMP_CRC32(crc, blk, blen);
blk += blen; blk += blen;
} }
/* Check that xl_tot_len agrees with our calculation */ /* 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), ereport(emode_for_corrupt_record(emode, recptr),
(errmsg("incorrect total length in record at %X/%X", (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. * 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 * 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. * 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) if (targetRecOff <= XLOG_BLCKSZ - SizeOfXLogRecord)
{ {