Re: Reducing the chunk header sizes on all memory context types - Mailing list pgsql-hackers

One more thing: based on what I saw in working with my pending guc.c
changes, the assertions in GetMemoryChunkMethodID are largely useless
for detecting bogus pointers.  I think we should do something more
like the attached, which will result in a clean failure if the method
ID bits are invalid.

I'm a little tempted also to rearrange the MemoryContextMethodID enum
so that likely bit patterns like 000 are not valid IDs.

While I didn't change it here, I wonder why GetMemoryChunkMethodID is
publicly exposed at all.  AFAICS it could be static inside mcxt.c.

            regards, tom lane

diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index c80236d285..e82d9b21ba 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -32,6 +32,11 @@
 #include "utils/memutils_internal.h"


+static void BogusFree(void *pointer);
+static void *BogusRealloc(void *pointer, Size size);
+static MemoryContext BogusGetChunkContext(void *pointer);
+static Size BogusGetChunkSpace(void *pointer);
+
 /*****************************************************************************
  *      GLOBAL MEMORY                                                             *
  *****************************************************************************/
@@ -74,10 +79,42 @@ static const MemoryContextMethods mcxt_methods[] = {
     [MCTX_SLAB_ID].get_chunk_context = SlabGetChunkContext,
     [MCTX_SLAB_ID].get_chunk_space = SlabGetChunkSpace,
     [MCTX_SLAB_ID].is_empty = SlabIsEmpty,
-    [MCTX_SLAB_ID].stats = SlabStats
+    [MCTX_SLAB_ID].stats = SlabStats,
 #ifdef MEMORY_CONTEXT_CHECKING
-    ,[MCTX_SLAB_ID].check = SlabCheck
+    [MCTX_SLAB_ID].check = SlabCheck,
 #endif
+
+    /*
+     * Unused (as yet) IDs should have dummy entries here.  This allows us to
+     * fail cleanly if a bogus pointer is passed to pfree or the like.  It
+     * seems sufficient to provide routines for the methods that might get
+     * invoked from inspection of a chunk (see MCXT_METHOD calls below).
+     */
+
+    [MCTX_UNUSED3_ID].free_p = BogusFree,
+    [MCTX_UNUSED3_ID].realloc = BogusRealloc,
+    [MCTX_UNUSED3_ID].get_chunk_context = BogusGetChunkContext,
+    [MCTX_UNUSED3_ID].get_chunk_space = BogusGetChunkSpace,
+
+    [MCTX_UNUSED4_ID].free_p = BogusFree,
+    [MCTX_UNUSED4_ID].realloc = BogusRealloc,
+    [MCTX_UNUSED4_ID].get_chunk_context = BogusGetChunkContext,
+    [MCTX_UNUSED4_ID].get_chunk_space = BogusGetChunkSpace,
+
+    [MCTX_UNUSED5_ID].free_p = BogusFree,
+    [MCTX_UNUSED5_ID].realloc = BogusRealloc,
+    [MCTX_UNUSED5_ID].get_chunk_context = BogusGetChunkContext,
+    [MCTX_UNUSED5_ID].get_chunk_space = BogusGetChunkSpace,
+
+    [MCTX_UNUSED6_ID].free_p = BogusFree,
+    [MCTX_UNUSED6_ID].realloc = BogusRealloc,
+    [MCTX_UNUSED6_ID].get_chunk_context = BogusGetChunkContext,
+    [MCTX_UNUSED6_ID].get_chunk_space = BogusGetChunkSpace,
+
+    [MCTX_UNUSED7_ID].free_p = BogusFree,
+    [MCTX_UNUSED7_ID].realloc = BogusRealloc,
+    [MCTX_UNUSED7_ID].get_chunk_context = BogusGetChunkContext,
+    [MCTX_UNUSED7_ID].get_chunk_space = BogusGetChunkSpace,
 };

 /*
@@ -125,6 +162,34 @@ static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
 #define MCXT_METHOD(pointer, method) \
     mcxt_methods[GetMemoryChunkMethodID(pointer)].method

+/*
+ * Support routines to trap use of invalid memory context method IDs
+ * (from calling pfree or the like on a bogus pointer).
+ */
+static void
+BogusFree(void *pointer)
+{
+    elog(ERROR, "pfree called with invalid pointer %p", pointer);
+}
+
+static void *
+BogusRealloc(void *pointer, Size size)
+{
+    elog(ERROR, "repalloc called with invalid pointer %p", pointer);
+}
+
+static MemoryContext
+BogusGetChunkContext(void *pointer)
+{
+    elog(ERROR, "GetMemoryChunkContext called with invalid pointer %p", pointer);
+}
+
+static Size
+BogusGetChunkSpace(void *pointer)
+{
+    elog(ERROR, "GetMemoryChunkSpace called with invalid pointer %p", pointer);
+}
+

 /*****************************************************************************
  *      EXPORTED ROUTINES                                                         *
diff --git a/src/include/utils/memutils_internal.h b/src/include/utils/memutils_internal.h
index 377cee7a84..797409ee94 100644
--- a/src/include/utils/memutils_internal.h
+++ b/src/include/utils/memutils_internal.h
@@ -77,12 +77,20 @@ extern void SlabCheck(MemoryContext context);
  *        The maximum value for this enum is constrained by
  *        MEMORY_CONTEXT_METHODID_MASK.  If an enum value higher than that is
  *        required then MEMORY_CONTEXT_METHODID_BITS will need to be increased.
+ *        For robustness, ensure that MemoryContextMethodID has a value for
+ *        each possible bit-pattern of MEMORY_CONTEXT_METHODID_MASK, and make
+ *        dummy entries for unused IDs in the mcxt_methods[] array.
  */
 typedef enum MemoryContextMethodID
 {
     MCTX_ASET_ID,
     MCTX_GENERATION_ID,
     MCTX_SLAB_ID,
+    MCTX_UNUSED3_ID,
+    MCTX_UNUSED4_ID,
+    MCTX_UNUSED5_ID,
+    MCTX_UNUSED6_ID,
+    MCTX_UNUSED7_ID
 } MemoryContextMethodID;

 /*
@@ -110,20 +118,11 @@ extern void MemoryContextCreate(MemoryContext node,
  *        directly precedes 'pointer'.
  */
 static inline MemoryContextMethodID
-GetMemoryChunkMethodID(void *pointer)
+GetMemoryChunkMethodID(const void *pointer)
 {
     uint64        header;

-    /*
-     * Try to detect bogus pointers handed to us, poorly though we can.
-     * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
-     * allocated chunk.
-     */
-    Assert(pointer != NULL);
-    Assert(pointer == (void *) MAXALIGN(pointer));
-
-    header = *((uint64 *) ((char *) pointer - sizeof(uint64)));
-
+    header = *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
     return (MemoryContextMethodID) (header & MEMORY_CONTEXT_METHODID_MASK);
 }