Fix some possibly latent bugs in slab.c

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 d5ee4db0e.

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
index 17aeaecac6fbf1ce5f376fc06ef78651003eb9d0..af12ef08696b7ccba5a74160c1fe2966c5d0fc24 100644 (file)
--- 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);
                }