postgres.git / summary / log / commit / refs

commit    6a33bb35c0411e298c99dfd4b40772c3f91f08ad
Author:   Tom Lane <tgl@sss.pgh.pa.us>
Date:     Thu Jan 30 20:36:07 2025 +0000

    Avoid integer overflow while testing wal_skip_threshold condition.
    
    smgrDoPendingSyncs had two distinct risks of integer overflow while
    deciding which way to ensure durability of a newly-created relation.
    First, it accumulated the total size of all forks in a variable of
    type BlockNumber (uint32).  While we restrict an individual fork's
    size to fit in that, I don't believe there's such a restriction on
    all of them added together.  Second, it proceeded to multiply the
    sum by BLCKSZ, which most certainly could overflow a uint32.
    
    (The exact expression is total_blocks * BLCKSZ / 1024.  The
    compiler might choose to optimize that to total_blocks * 8,
    which is not at quite as much risk of overflow as a literal
    reading would be, but it's still wrong.)
    
    If an overflow did occur it could lead to a poor choice to
    shove a very large relation into WAL instead of fsync'ing it.
    This wouldn't be fatal, but it could be inefficient.
    
    Change total_blocks to uint64 which should be plenty, and
    rearrange the comparison calculation to be overflow-safe.
    
    I noticed this while looking for ramifications of the proposed
    change in MAX_KILOBYTES.  It's not entirely clear to me why
    wal_skip_threshold is limited to MAX_KILOBYTES in the
    first place, but in any case this code is unsafe regardless
    of the range of wal_skip_threshold.
    
    Oversight in c6b92041d which introduced wal_skip_threshold,
    so back-patch to v13.
    
    Discussion: https://postgr.es/m/1a01f0-66ec2d80-3b-68487680@27595217
    Backpatch-through: 13


 src/backend/catalog/storage.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index f563f2a1690..f5d566dac87 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -773,7 +773,7 @@ smgrDoPendingSyncs(bool isCommit, bool isParallelWorker)
 	{
 		ForkNumber	fork;
 		BlockNumber nblocks[MAX_FORKNUM + 1];
-		BlockNumber total_blocks = 0;
+		uint64		total_blocks = 0;
 		SMgrRelation srel;
 
 		srel = smgropen(pendingsync->rnode, InvalidBackendId);
@@ -817,7 +817,7 @@ smgrDoPendingSyncs(bool isCommit, bool isParallelWorker)
 		 * main fork is longer than ever but FSM fork gets shorter.
 		 */
 		if (pendingsync->is_truncated ||
-			total_blocks * BLCKSZ / 1024 >= wal_skip_threshold)
+			total_blocks >= wal_skip_threshold * (uint64) 1024 / BLCKSZ)
 		{
 			/* allocate the initial array, or extend it, if needed */
 			if (maxrels == 0)


[parent: c05268e6ec93]