On Tue, 6 Sept 2022 at 12:27, Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowle...@gmail.com> writes:
> > On Tue, 6 Sept 2022 at 11:09, Andres Freund <and...@anarazel.de> wrote:
> >> I was looking at
> >> MemoryContextContains(). Unless I am missing something, the patch omitted
> >> adjusting that? We'll probably always return false right now.
>
> > Oops. Yes. I'll push a fix a bit later.
I think the fix is harder than I thought, or perhaps impossible to do
given how we now determine the owning MemoryContext of a pointer.
There's a comment in MemoryContextContains which says:
* 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.
That seems to indicate that we should be able to handle any random
pointer given to us (!). That comment seems more confident that'll
work than the function's header comment does:
* 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.
Here that's just claiming the test might not be reliable and could
return false-positive results.
I find this entire function pretty scary as even before the context
changes that function seems to think it's fine to subtract sizeof(void
*) from the given pointer and dereference that memory. That could very
well segfault.
I wonder if there are many usages of MemoryContextContains in
extensions. If there's not, I'd be much happier if we got rid of this
function and used GetMemoryChunkContext() in nodeAgg.c and
nodeWindowAgg.c.
> +1 for adding something to regress.c that verifies that this
> works properly for all three allocators. I suggest making
> three contexts and cross-checking the correct results for
> all combinations of chunk A vs context B.
I went as far as adding an Assert to palloc(). I'm not quite sure what
you have in mind in regress.c
Attached is a draft patch. I just don't like this function one bit.
David
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 64bcc7ef32..a6b2d02b75 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -809,11 +809,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,10 +820,6 @@ 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.
- *
* 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.
@@ -835,7 +830,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;
}
@@ -1151,6 +1146,8 @@ palloc(Size size)
size, context->name)));
}
+ Assert(MemoryContextContains(context, ret));
+
VALGRIND_MEMPOOL_ALLOC(context, ret, size);
return ret;
