Potential stack overflow in incremental base backup

From: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
To: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Potential stack overflow in incremental base backup
Date: 2024-03-06 05:43:36
Message-ID: CA+hUKG+2hZ0sBztPW4mkLfng0qfkNtAHFUfxOMLizJ0BPmi5+g@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Robert,

I was rebasing my patch to convert RELSEG_SIZE into an initdb-time
setting, and thus runtime variable, and I noticed this stack variable
in src/backend/backup/basebackup_incremental.c:

GetFileBackupMethod(IncrementalBackupInfo *ib, const char *path,
BlockNumber *relative_block_numbers,
unsigned *truncation_block_length)
{
BlockNumber absolute_block_numbers[RELSEG_SIZE];

I'll have to move that sucker onto the heap (we banned C99 variable
length arrays and we don't use nonstandard alloca()), but I think the
coding in master is already a bit dodgy: that's 131072 *
sizeof(BlockNumber) = 512kB with default configure options, but
--with-segsize X could create a stack variable up to 16GB,
corresponding to segment size 32TB (meaning no segmentation at all).
That wouldn't work. Shouldn't we move it to the stack? See attached
draft patch.

Even on the heap, 16GB is too much to assume we can allocate during a
base backup. I don't claim that's a real-world problem for
incremental backup right now in master, because I don't have any
evidence that anyone ever really uses --with-segsize (do they?), but
if we make it an initdb option it will be more popular and this will
become a problem. Hmm.

Attachment Content-Type Size
0001-Fix-potential-stack-overflow-in-incremental-baseback.patch text/x-patch 2.4 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2024-03-06 05:47:55 Re: Fix race condition in InvalidatePossiblyObsoleteSlot()
Previous Message shveta malik 2024-03-06 04:54:46 Re: Missing LWLock protection in pgstat_reset_replslot()