Improve our ability to detect bogus pointers passed to pfree et al.

Commit c6e0fe1f2 was a shade too trusting that any pointer passed
to pfree, repalloc, etc will point at a valid chunk.  Notably,
passing a pointer that was actually obtained from malloc tended
to result in obscure assertion failures, if not worse.  (On FreeBSD
I've seen such mistakes take down the entire cluster, seemingly as
a result of clobbering shared memory.)

To improve matters, extend the mcxt_methods[] array so that it
has entries for every possible MemoryContextMethodID bit-pattern,
with the currently unassigned ID codes pointing to error-reporting
functions.  Then, fiddle with the ID assignments so that patterns
likely to be associated with bad pointers aren't valid ID codes.
In particular, we should avoid assigning bit patterns 000 (zeroed
memory) and 111 (wipe_mem'd memory).

It turns out that on glibc (Linux), malloc uses chunk headers that
have flag bits in the same place we keep MemoryContextMethodID,
and that the bit patterns 000, 001, 010 are the only ones we'll
see as long as the backend isn't threaded.  So we can have very
robust detection of pfree'ing a malloc-assigned block on that
platform, at least so long as we can refrain from using up those
ID codes.  On other platforms, we don't have such a good guarantee,
but keeping 000 reserved will be enough to catch many such cases.

While here, make GetMemoryChunkMethodID() local to mcxt.c, as there
seems no need for it to be exposed even in memutils_internal.h.

Patch by me, with suggestions from Andres Freund and David Rowley.

Discussion: https://postgr.es/m/2910981.1665080361@sss.pgh.pa.us

diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index c80236d285ae6e421d7bbeb8b39aa3cae8305a28..b1a3c74830575f452a7be169202c15da93d42ec6 100644 (file)
--- 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_UNUSED1_ID].free_p = BogusFree,
+   [MCTX_UNUSED1_ID].realloc = BogusRealloc,
+   [MCTX_UNUSED1_ID].get_chunk_context = BogusGetChunkContext,
+   [MCTX_UNUSED1_ID].get_chunk_space = BogusGetChunkSpace,
+
+   [MCTX_UNUSED2_ID].free_p = BogusFree,
+   [MCTX_UNUSED2_ID].realloc = BogusRealloc,
+   [MCTX_UNUSED2_ID].get_chunk_context = BogusGetChunkContext,
+   [MCTX_UNUSED2_ID].get_chunk_space = BogusGetChunkSpace,
+
+   [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,
 };
 
 /*
@@ -125,6 +162,77 @@ static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
 #define MCXT_METHOD(pointer, method) \
    mcxt_methods[GetMemoryChunkMethodID(pointer)].method
 
+/*
+ * GetMemoryChunkMethodID
+ *     Return the MemoryContextMethodID from the uint64 chunk header which
+ *     directly precedes 'pointer'.
+ */
+static inline MemoryContextMethodID
+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 == (const void *) MAXALIGN(pointer));
+
+   header = *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+
+   return (MemoryContextMethodID) (header & MEMORY_CONTEXT_METHODID_MASK);
+}
+
+/*
+ * GetMemoryChunkHeader
+ *     Return the uint64 chunk header which directly precedes 'pointer'.
+ *
+ * This is only used after GetMemoryChunkMethodID, so no need for error checks.
+ */
+static inline uint64
+GetMemoryChunkHeader(const void *pointer)
+{
+   return *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+}
+
+/*
+ * Support routines to trap use of invalid memory context method IDs
+ * (from calling pfree or the like on a bogus pointer).  As a possible
+ * aid in debugging, we report the header word along with the pointer
+ * address (if we got here, there must be an accessible header word).
+ */
+static void
+BogusFree(void *pointer)
+{
+   elog(ERROR, "pfree called with invalid pointer %p (header 0x%016llx)",
+        pointer, (long long) GetMemoryChunkHeader(pointer));
+}
+
+static void *
+BogusRealloc(void *pointer, Size size)
+{
+   elog(ERROR, "repalloc called with invalid pointer %p (header 0x%016llx)",
+        pointer, (long long) GetMemoryChunkHeader(pointer));
+   return NULL;                /* keep compiler quiet */
+}
+
+static MemoryContext
+BogusGetChunkContext(void *pointer)
+{
+   elog(ERROR, "GetMemoryChunkContext called with invalid pointer %p (header 0x%016llx)",
+        pointer, (long long) GetMemoryChunkHeader(pointer));
+   return NULL;                /* keep compiler quiet */
+}
+
+static Size
+BogusGetChunkSpace(void *pointer)
+{
+   elog(ERROR, "GetMemoryChunkSpace called with invalid pointer %p (header 0x%016llx)",
+        pointer, (long long) GetMemoryChunkHeader(pointer));
+   return 0;                   /* keep compiler quiet */
+}
+
 
 /*****************************************************************************
  *   EXPORTED ROUTINES                                                      *

diff --git a/src/include/utils/memutils_internal.h b/src/include/utils/memutils_internal.h
index 377cee7a841003f5aeaec23094b9b75657e9fff3..bc2cbdd5061eeb9af9089f106305308cbb1f9cc7 100644 (file)
--- a/src/include/utils/memutils_internal.h
+++ b/src/include/utils/memutils_internal.h
@@ -74,15 +74,26 @@ extern void SlabCheck(MemoryContext context);
  * MemoryContextMethodID
  *     A unique identifier for each MemoryContext implementation which
  *     indicates the index into the mcxt_methods[] array. See mcxt.c.
- *     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 robust error detection, 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.  We also try
+ * to avoid using bit-patterns as valid IDs if they are likely to occur in
+ * garbage data, or if they could falsely match on chunks that are really from
+ * malloc not palloc.  (We can't tell that for most malloc implementations,
+ * but it happens that glibc stores flag bits in the same place where we put
+ * the MemoryContextMethodID, so the possible values are predictable for it.)
  */
 typedef enum MemoryContextMethodID
 {
+   MCTX_UNUSED1_ID,            /* 000 occurs in never-used memory */
+   MCTX_UNUSED2_ID,            /* glibc malloc'd chunks usually match 001 */
+   MCTX_UNUSED3_ID,            /* glibc malloc'd chunks > 128kB match 010 */
    MCTX_ASET_ID,
    MCTX_GENERATION_ID,
    MCTX_SLAB_ID,
+   MCTX_UNUSED4_ID,            /* available */
+   MCTX_UNUSED5_ID             /* 111 occurs in wipe_mem'd memory */
 } MemoryContextMethodID;
 
 /*
@@ -104,27 +115,4 @@ extern void MemoryContextCreate(MemoryContext node,
                                MemoryContext parent,
                                const char *name);
 
-/*
- * GetMemoryChunkMethodID
- *     Return the MemoryContextMethodID from the uint64 chunk header which
- *     directly precedes 'pointer'.
- */
-static inline MemoryContextMethodID
-GetMemoryChunkMethodID(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)));
-
-   return (MemoryContextMethodID) (header & MEMORY_CONTEXT_METHODID_MASK);
-}
-
 #endif                         /* MEMUTILS_INTERNAL_H */