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 |
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() |