On Fri, Apr 07, 2023 at 08:08:34AM +0900, Michael Paquier wrote: > So bumping mainrdata_len to uint64 is actually not entirely in line > with this code. Well, it will work because we'd still fail a couple > of lines down, but perhaps its readability should be improved so as > we have an extra check in this code path to make sure that > mainrdata_len is not higher than PG_UINT32_MAX, then use an > intermediate casted variable before saving the length in the record > data to make clear that the type of the main static length in > xloginsert.c is not the same as what a record has? The v10 I sent > previously blocked this possibility, but not v11.
So, I was thinking about something like the attached tweaking this point, the error details a bit, applying an indentation and writing a commit message... Matthias? -- Michael
From 10eff110176a4988295118b0f17a2a868fb9b96e Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Fri, 7 Apr 2023 08:19:24 +0900 Subject: [PATCH v12] Add more protections in WAL record APIs against overflows This commit adds a limit to the size of an XLogRecord at 1020MiB, based on a suggestion by Heikki Linnakangas. This counts for the overhead needed by the xlogreader when allocating the memory it needs to read a record in DecodeXLogRecordRequiredSpace(). An assertion based on that is added to make sure to detect that any additions in the XLogReader facilities would not cause any overflows. If that's ever the case, the upper bound would need to be adjusted. Before this, it was possible for an external module to create WAL records large enough to be assembled still not replayable, causing failures when replaying such WAL records on standbys. One case mentioned where this is possible is the in-core function pg_logical_emit_message(), that allows to emit WAL records with an arbitrary amount of data up to 2GB, much higher than the replay limit of approximately 1GB (limit of a palloc). This commit is a follow-up of ffd1b6b that has added similar protections for the block-level data. Here, the checks are extended to the whole record length. All the error messages related to overflow checks are improved to provide more context about what is happening. Author: Matthias van de Meent Discussion: https://postgr.es/m/CAEze2WgGiw+LZt+vHf8tWqB_6VxeLsMeoAuod0N=ij1q17n...@mail.gmail.com --- src/include/access/xlogrecord.h | 11 +++++ src/backend/access/transam/xloginsert.c | 60 ++++++++++++++++++++++--- 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/src/include/access/xlogrecord.h b/src/include/access/xlogrecord.h index 0d576f7883..f355e08e1d 100644 --- a/src/include/access/xlogrecord.h +++ b/src/include/access/xlogrecord.h @@ -62,6 +62,17 @@ typedef struct XLogRecord #define XLR_INFO_MASK 0x0F #define XLR_RMGR_INFO_MASK 0xF0 +/* + * XLogReader needs to allocate all the data of a WAL record in a single + * chunk. This means that a single XLogRecord cannot exceed MaxAllocSize + * in length if we ignore any allocation overhead of the XLogReader. + * + * To accommodate some overhead, this value allows for 4M of allocation + * overhead, that should be plenty enough for what + * DecodeXLogRecordRequiredSpace() expects as extra. + */ +#define XLogRecordMaxSize (1020 * 1024 * 1024) + /* * If a WAL record modifies any relation files, in ways not covered by the * usual block references, this flag is set. This is not used for anything diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c index 008612e032..b71e690c5b 100644 --- a/src/backend/access/transam/xloginsert.c +++ b/src/backend/access/transam/xloginsert.c @@ -98,7 +98,7 @@ static int max_registered_block_id = 0; /* highest block_id + 1 currently */ static XLogRecData *mainrdata_head; static XLogRecData *mainrdata_last = (XLogRecData *) &mainrdata_head; -static uint32 mainrdata_len; /* total # of bytes in chain */ +static uint64 mainrdata_len; /* total # of bytes in chain */ /* flags for the in-progress insertion */ static uint8 curinsert_flags = 0; @@ -355,7 +355,10 @@ XLogRegisterData(char *data, uint32 len) Assert(begininsert_called); if (num_rdatas >= max_rdatas) - elog(ERROR, "too much WAL data"); + ereport(ERROR, + (errmsg_internal("too much WAL data"), + errdetail_internal("%u out of %u data segments are already in use.", + num_rdatas, max_rdatas))); rdata = &rdatas[num_rdatas++]; rdata->data = data; @@ -405,9 +408,16 @@ XLogRegisterBufData(uint8 block_id, char *data, uint32 len) * regbuf->rdata_len does not grow beyond what * XLogRecordBlockHeader->data_length can hold. */ - if (num_rdatas >= max_rdatas || - regbuf->rdata_len + len > UINT16_MAX) - elog(ERROR, "too much WAL data"); + if (num_rdatas >= max_rdatas) + ereport(ERROR, + (errmsg_internal("too much WAL data"), + errdetail_internal("%u out of %u data segments are already in use.", + num_rdatas, max_rdatas))); + if (regbuf->rdata_len + len > UINT16_MAX || len > UINT16_MAX) + ereport(ERROR, + (errmsg_internal("too much WAL data"), + errdetail_internal("Registering more than max %u bytes allowed to block %u: current %u bytes, adding %u bytes.", + UINT16_MAX, block_id, regbuf->rdata_len, len))); rdata = &rdatas[num_rdatas++]; @@ -527,7 +537,7 @@ XLogRecordAssemble(RmgrId rmid, uint8 info, XLogRecPtr *fpw_lsn, int *num_fpi, bool *topxid_included) { XLogRecData *rdt; - uint32 total_len = 0; + uint64 total_len = 0; int block_id; pg_crc32c rdata_crc; registered_buffer *prev_regbuf = NULL; @@ -841,8 +851,18 @@ XLogRecordAssemble(RmgrId rmid, uint8 info, { if (mainrdata_len > 255) { + uint32 mainrdata_len_4b; + + if (mainrdata_len > PG_UINT32_MAX) + ereport(ERROR, + (errmsg_internal("too much WAL data"), + errdetail_internal("Main data length at %llu bytes for a max of %u bytes.", + (unsigned long long) mainrdata_len, + PG_UINT32_MAX))); + + mainrdata_len_4b = (uint32) mainrdata_len; *(scratch++) = (char) XLR_BLOCK_ID_DATA_LONG; - memcpy(scratch, &mainrdata_len, sizeof(uint32)); + memcpy(scratch, &mainrdata_len_4b, sizeof(uint32)); scratch += sizeof(uint32); } else @@ -872,6 +892,20 @@ XLogRecordAssemble(RmgrId rmid, uint8 info, for (rdt = hdr_rdt.next; rdt != NULL; rdt = rdt->next) COMP_CRC32C(rdata_crc, rdt->data, rdt->len); + /* + * Ensure that the XLogRecord is not too large. + * + * XLogReader machinery is only able to handle records up to a certain + * size (ignoring machine resource limitations), so make sure that we will + * not emit records larger than the sizes advertised to be supported. + * This cap is based on DecodeXLogRecordRequiredSpace(). + */ + if (total_len >= XLogRecordMaxSize) + ereport(ERROR, + (errmsg_internal("oversized WAL record"), + errdetail_internal("WAL record would be %llu bytes (of max %u bytes); rmid %u flags %u.", + (unsigned long long) total_len, XLogRecordMaxSize, rmid, info))); + /* * Fill in the fields in the record header. Prev-link is filled in later, * once we know where in the WAL the record will be inserted. The CRC does @@ -1297,6 +1331,18 @@ log_newpage_range(Relation rel, ForkNumber forknum, void InitXLogInsert(void) { +#ifdef USE_ASSERT_CHECKING + + /* + * Check that any records assembled can be decoded. This is capped based + * on what XLogReader would require at its maximum bound. This code path + * is called once per backend, more than enough for this check. + */ + size_t max_required = DecodeXLogRecordRequiredSpace(XLogRecordMaxSize); + + Assert(AllocSizeIsValid(max_required)); +#endif + /* Initialize the working areas */ if (xloginsert_cxt == NULL) { -- 2.40.0
signature.asc
Description: PGP signature
