Re: Reducing the chunk header sizes on all memory context types

[David Rowley]

On Tue, 6 Sept 2022 at 14:43, Tom Lane <t...@sss.pgh.pa.us> wrote:
> I think MemoryContextContains' charter is to return
>
>         GetMemoryChunkContext(pointer) == context
>
> *except* that instead of asserting what GetMemoryChunkContext asserts,
> it should treat those cases as reasons to return false.  So if you
> can still do GetMemoryChunkContext then you can still do
> MemoryContextContains.  The point of having the separate function
> is to be as forgiving as we can of bogus pointers.

Ok.  I've readded the Asserts that c6e0fe1f2 mistakenly removed from
GetMemoryChunkContext() and changed MemoryContextContains() to do
those same pre-checks before calling GetMemoryChunkContext().

I've also boosted the Assert in mcxt.c to
Assert(MemoryContextContains(context, ret)) in each place we call the
context's callback function to obtain a newly allocated pointer.  I
think this should cover the testing.

I felt the need to keep the adjustments I made to the header comment
in MemoryContextContains() to ward off anyone who thinks it's ok to
pass this any random pointer and have it do something sane. It's much
more prone to misbehaving/segfaulting now given the extra dereferences
that c6e0fe1f2 added to obtain a pointer to the owning context.

David

diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 64bcc7ef32..3d80abbfae 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -482,6 +482,15 @@ MemoryContextAllowInCriticalSection(MemoryContext context, 
bool allow)
 MemoryContext
 GetMemoryChunkContext(void *pointer)
 {
+       /*
+        * 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));
+       /* adding further Asserts here? See pre-checks in MemoryContextContains 
*/
+
        return MCXT_METHOD(pointer, get_chunk_context) (pointer);
 }
 
@@ -809,11 +818,10 @@ MemoryContextCheck(MemoryContext context)
  *             Detect whether an allocated chunk of memory belongs to a given
  *             context or not.
  *
- * Caution: this test is reliable as long as 'pointer' does point to
- * a chunk of memory allocated from *some* context.  If 'pointer' points
- * at memory obtained in some other way, there is a small chance of a
- * false-positive result, since the bits right before it might look like
- * a valid chunk header by chance.
+ * Caution: 'pointer' must point to a pointer which was allocated by a
+ * MemoryContext.  It's not safe or valid to use this function on arbitrary
+ * pointers as obtaining the MemoryContext which 'pointer' belongs to requires
+ * possibly several pointer dereferences.
  */
 bool
 MemoryContextContains(MemoryContext context, void *pointer)
@@ -821,9 +829,8 @@ MemoryContextContains(MemoryContext context, void *pointer)
        MemoryContext ptr_context;
 
        /*
-        * NB: Can't use GetMemoryChunkContext() here - that performs assertions
-        * that aren't acceptable here since we might be passed memory not
-        * allocated by any memory context.
+        * NB: We must perform run-time checks here which 
GetMemoryChunkContext()
+        * does as assertions before calling GetMemoryChunkContext().
         *
         * Try to detect bogus pointers handed to us, poorly though we can.
         * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
@@ -835,7 +842,7 @@ MemoryContextContains(MemoryContext context, void *pointer)
        /*
         * OK, it's probably safe to look at the context.
         */
-       ptr_context = *(MemoryContext *) (((char *) pointer) - sizeof(void *));
+       ptr_context = GetMemoryChunkContext(pointer);
 
        return ptr_context == context;
 }
@@ -953,6 +960,8 @@ MemoryContextAlloc(MemoryContext context, Size size)
 
        VALGRIND_MEMPOOL_ALLOC(context, ret, size);
 
+       Assert(MemoryContextContains(context, ret));
+
        return ret;
 }
 
@@ -991,6 +1000,8 @@ MemoryContextAllocZero(MemoryContext context, Size size)
 
        MemSetAligned(ret, 0, size);
 
+       Assert(MemoryContextContains(context, ret));
+
        return ret;
 }
 
@@ -1029,6 +1040,8 @@ MemoryContextAllocZeroAligned(MemoryContext context, Size 
size)
 
        MemSetLoop(ret, 0, size);
 
+       Assert(MemoryContextContains(context, ret));
+
        return ret;
 }
 
@@ -1070,6 +1083,8 @@ MemoryContextAllocExtended(MemoryContext context, Size 
size, int flags)
        if ((flags & MCXT_ALLOC_ZERO) != 0)
                MemSetAligned(ret, 0, size);
 
+       Assert(MemoryContextContains(context, ret));
+
        return ret;
 }
 
@@ -1153,6 +1168,8 @@ palloc(Size size)
 
        VALGRIND_MEMPOOL_ALLOC(context, ret, size);
 
+       Assert(MemoryContextContains(context, ret));
+
        return ret;
 }
 
@@ -1186,6 +1203,8 @@ palloc0(Size size)
 
        MemSetAligned(ret, 0, size);
 
+       Assert(MemoryContextContains(context, ret));
+
        return ret;
 }
 
@@ -1225,6 +1244,8 @@ palloc_extended(Size size, int flags)
        if ((flags & MCXT_ALLOC_ZERO) != 0)
                MemSetAligned(ret, 0, size);
 
+       Assert(MemoryContextContains(context, ret));
+
        return ret;
 }
 
@@ -1278,6 +1299,8 @@ repalloc(void *pointer, Size size)
 
        VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size);
 
+       Assert(MemoryContextContains(context, ret));
+
        return ret;
 }
 
@@ -1313,6 +1336,8 @@ MemoryContextAllocHuge(MemoryContext context, Size size)
 
        VALGRIND_MEMPOOL_ALLOC(context, ret, size);
 
+       Assert(MemoryContextContains(context, ret));
+
        return ret;
 }
 
@@ -1352,6 +1377,8 @@ repalloc_huge(void *pointer, Size size)
 
        VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size);
 
+       Assert(MemoryContextContains(context, ret));
+
        return ret;
 }