Fix some possibly latent bugs in slab.c

david-rowley · david-rowley · commit f249f1026f71 · 2022-09-01T19:23:44.000+12:00
Primarily, this fixes an incorrect calculation in SlabCheck which was looking in the wrong byte for the sentinel check. The reason that we've never noticed this before in the form of a failing sentinel check is because the pre-check to this always fails because all current core users of slab contexts have a chunk size which is already MAXALIGNed, therefore there's never any space for the sentinel byte. It is possible that an extension needs to use a slab context and if they do with a chunk size that's not MAXALIGNed, then they'll likely get errors about overwritten sentinel bytes. Additionally, this patch changes various calculations which are being done based on the sizeof(SlabBlock). Currently, sizeof(SlabBlock) is a multiple of 8, therefore sizeof(SlabBlock) is the same as MAXALIGN(sizeof(SlabBlock)), however, if we were to ever have to add any fields to that struct as part of a bug fix, then SlabAlloc could end up returning a non-MAXALIGNed pointer. To be safe, let's ensure we always MAXALIGN sizeof(SlabBlock) before using it in any calculations. This patch has already been applied to master in d5ee4db. Diagnosed-by: Tomas Vondra, Tom Lane Author: Tomas Vondra, David Rowley Discussion: https://postgr.es/m/CAA4eK1%2B1JyW5TiL%3DyV-3Uq1CrfnTyn0Xrk5uArt31Z%3D8rgPhXQ%40mail.gmail.com Backpatch-through: 10
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
@@ -57,6 +57,8 @@
 #include "lib/ilist.h"
 
 
+#define Slab_BLOCKHDRSZ	MAXALIGN(sizeof(SlabBlock))
+
 /*
  * SlabContext is a specialized implementation of MemoryContext.
  */
@@ -117,10 +119,10 @@ typedef struct SlabChunk
 #define SlabChunkGetPointer(chk)	\
 	((void *)(((char *)(chk)) + sizeof(SlabChunk)))
 #define SlabBlockGetChunk(slab, block, idx) \
-	((SlabChunk *) ((char *) (block) + sizeof(SlabBlock)	\
+	((SlabChunk *) ((char *) (block) + Slab_BLOCKHDRSZ	\
 					+ (idx * slab->fullChunkSize)))
 #define SlabBlockStart(block)	\
-	((char *) block + sizeof(SlabBlock))
+	((char *) block + Slab_BLOCKHDRSZ)
 #define SlabChunkIndex(slab, block, chunk)	\
 	(((char *) chunk - SlabBlockStart(block)) / slab->fullChunkSize)
 
@@ -185,7 +187,7 @@ static const MemoryContextMethods SlabMethods = {
  * chunkSize: allocation chunk size
  *
  * The chunkSize may not exceed:
- *		MAXALIGN_DOWN(SIZE_MAX) - MAXALIGN(sizeof(SlabBlock)) - SLAB_CHUNKHDRSZ
+ *		MAXALIGN_DOWN(SIZE_MAX) - MAXALIGN(Slab_BLOCKHDRSZ) - sizeof(SlabChunk)
  */
 MemoryContext
 SlabContextCreate(MemoryContext parent,
@@ -215,12 +217,12 @@ SlabContextCreate(MemoryContext parent,
 	fullChunkSize = sizeof(SlabChunk) + MAXALIGN(chunkSize);
 
 	/* Make sure the block can store at least one chunk. */
-	if (blockSize < fullChunkSize + sizeof(SlabBlock))
+	if (blockSize < fullChunkSize + Slab_BLOCKHDRSZ)
 		elog(ERROR, "block size %zu for slab is too small for %zu chunks",
 			 blockSize, chunkSize);
 
 	/* Compute maximum number of chunks per block */
-	chunksPerBlock = (blockSize - sizeof(SlabBlock)) / fullChunkSize;
+	chunksPerBlock = (blockSize - Slab_BLOCKHDRSZ) / fullChunkSize;
 
 	/* The freelist starts with 0, ends with chunksPerBlock. */
 	freelistSize = sizeof(dlist_head) * (chunksPerBlock + 1);
@@ -781,7 +783,7 @@ SlabCheck(MemoryContext context)
 
 					/* there might be sentinel (thanks to alignment) */
 					if (slab->chunkSize < (slab->fullChunkSize - sizeof(SlabChunk)))
-						if (!sentinel_ok(chunk, slab->chunkSize))
+						if (!sentinel_ok(chunk, sizeof(SlabChunk) + slab->chunkSize))
 							elog(WARNING, "problem in slab %s: detected write past chunk end in block %p, chunk %p",
 								 name, block, chunk);
 				}
