From d8f5acbdb9b22106db583e3cbb177d34e6b18eeb Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Thu, 11 Apr 2024 13:19:29 +1200 Subject: [PATCH] Fix potential stack overflow in incremental backup. The user can set RELSEG_SIZE to a high number at compile time, so we can't use it to control the size of an array on the stack: it could be many gigabytes in size. On closer inspection, we don't really need that intermediate array anyway. Let's just write directly into the output array, and then perform the absolute->relative adjustment in place. This fixes new code from commit dc212340058. Reviewed-by: Robert Haas Discussion: https://postgr.es/m/CA%2BhUKG%2B2hZ0sBztPW4mkLfng0qfkNtAHFUfxOMLizJ0BPmi5%2Bg%40mail.gmail.com --- src/backend/backup/basebackup_incremental.c | 26 ++++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c index 131598bade..957b70b692 100644 --- a/src/backend/backup/basebackup_incremental.c +++ b/src/backend/backup/basebackup_incremental.c @@ -726,7 +726,8 @@ GetIncrementalFilePath(Oid dboid, Oid spcoid, RelFileNumber relfilenumber, * How should we back up a particular file as part of an incremental backup? * * If the return value is BACK_UP_FILE_FULLY, caller should back up the whole - * file just as if this were not an incremental backup. + * file just as if this were not an incremental backup. The contents of the + * relative_block_numbers array is unspecified in this case. * * If the return value is BACK_UP_FILE_INCREMENTALLY, caller should include * an incremental file in the backup instead of the entire file. On return, @@ -745,7 +746,6 @@ GetFileBackupMethod(IncrementalBackupInfo *ib, const char *path, BlockNumber *relative_block_numbers, unsigned *truncation_block_length) { - BlockNumber absolute_block_numbers[RELSEG_SIZE]; BlockNumber limit_block; BlockNumber start_blkno; BlockNumber stop_blkno; @@ -872,8 +872,13 @@ GetFileBackupMethod(IncrementalBackupInfo *ib, const char *path, errcode(ERRCODE_INTERNAL_ERROR), errmsg_internal("overflow computing block number bounds for segment %u with size %zu", segno, size)); + + /* + * This will write *absolute* block numbers into the output array, but + * we'll transpose them below. + */ nblocks = BlockRefTableEntryGetBlocks(brtentry, start_blkno, stop_blkno, - absolute_block_numbers, RELSEG_SIZE); + relative_block_numbers, RELSEG_SIZE); Assert(nblocks <= RELSEG_SIZE); /* @@ -892,19 +897,22 @@ GetFileBackupMethod(IncrementalBackupInfo *ib, const char *path, return BACK_UP_FILE_FULLY; /* - * Looks like we can send an incremental file, so sort the absolute the - * block numbers and then transpose absolute block numbers to relative - * block numbers. + * Looks like we can send an incremental file, so sort the block numbers + * and then transpose them from absolute block numbers to relative block + * numbers if necessary. * * NB: If the block reference table was using the bitmap representation * for a given chunk, the block numbers in that chunk will already be * sorted, but when the array-of-offsets representation is used, we can * receive block numbers here out of order. */ - qsort(absolute_block_numbers, nblocks, sizeof(BlockNumber), + qsort(relative_block_numbers, nblocks, sizeof(BlockNumber), compare_block_numbers); - for (i = 0; i < nblocks; ++i) - relative_block_numbers[i] = absolute_block_numbers[i] - start_blkno; + if (start_blkno != 0) + { + for (i = 0; i < nblocks; ++i) + relative_block_numbers[i] -= start_blkno; + } *num_blocks_required = nblocks; /*