Re: Non-replayable WAL records through overflows and >MaxAllocSize lengths

On 11/03/2022 17:42, Matthias van de Meent wrote:
> Hi,
>
> Xlogreader limits the size of what it considers valid xlog records to
> MaxAllocSize; but this is not currently enforced in the
> XLogRecAssemble API. This means it is possible to assemble a record
> that postgresql cannot replay.

Oops, that would be nasty.

> Similarly; it is possible to repeatedly call XlogRegisterData() so as
> to overflow rec->xl_tot_len; resulting in out-of-bounds reads and
> writes while processing record data;

And that too.

Have you been able to create a test case for that? The largest record I
can think of is a commit record with a huge number of subtransactions,
dropped relations, and shared inval messages. I'm not sure if you can
overflow a uint32 with that, but exceeding MaxAllocSize seems possible.

> PFA a patch that attempts to fix both of these issues in the insertion
> API; by checking against overflows and other incorrectly large values
> in the relevant functions in xloginsert.c. In this patch, I've also
> added a comment to the XLogRecord spec to document that xl_tot_len
> should not be larger than 1GB - 1B; and why that limit exists.
> diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
> index c260310c4c..ae654177de 100644
> --- a/src/backend/access/transam/xloginsert.c
> +++ b/src/backend/access/transam/xloginsert.c
> @@ -342,6 +342,11 @@ XLogRegisterData(char *data, int len)
>
> if (num_rdatas >= max_rdatas)
> elog(ERROR, "too much WAL data");
> +
> + /* protect against overflow */
> + if (unlikely((uint64) mainrdata_len + (uint64) len > UINT32_MAX))
> + elog(ERROR, "too much WAL data");
> +
> rdata = &rdatas[num_rdatas++];
>
> rdata->data = data;

Could check for just AllocSizeValid(mainrdata_len), if we're only
worried about the total size of the data to exceed the limit, and assume
that each individual piece of data is smaller.

We also don't check for negative 'len'. I think that's fine, the caller
bears some responsibility for passing valid arguments too. But maybe
uint32 or size_t would be more appropriate here.

I wonder if these checks hurt performance. These are very cheap, but
then again, this codepath is very hot. It's probably fine, but it still
worries me a little. Maybe some of these could be Asserts.

> @@ -387,6 +392,11 @@ XLogRegisterBufData(uint8 block_id, char *data, int len)
>
> if (num_rdatas >= max_rdatas)
> elog(ERROR, "too much WAL data");
> +
> + /* protect against overflow */
> + if (unlikely((uint64) regbuf->rdata_len + (uint64) len > UINT32_MAX))
> + elog(ERROR, "too much WAL data");
> +
> rdata = &rdatas[num_rdatas++];
>
> rdata->data = data;

Could check "len > UINT16_MAX". As you noted in XLogRecordAssemble,
that's the real limit. And if you check for that here, you don't need to
check it in XLogRecordAssemble.

> @@ -505,7 +515,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;

I don't think the change to uint64 is necessary. If all the data blocks
are limited to 64 kB, and the number of blocks is limited, and the
number of blocks is limited too.

> @@ -734,6 +744,10 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
>
> if (needs_data)
> {
> + /* protect against overflow */
> + if (unlikely(regbuf->rdata_len > UINT16_MAX))
> + elog(ERROR, "too much WAL data for registered buffer");
> +
> /*
> * Link the caller-supplied rdata chain for this buffer to the
> * overall list.
> @@ -836,6 +850,13 @@ 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 xlogreader.c can read the record; and check that we don't
> + * accidentally overflow the size of the record.
> + * */
> + if (unlikely(!AllocSizeIsValid(total_len) || total_len > UINT32_MAX))
> + elog(ERROR, "too much registered data for WAL record");
> +
> /*
> * 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

It's enough to check AllocSizeIsValid(total_len), no need to also check
against UINT32_MAX.

- Heikki