Hi,
(really need to fix my mobile phone mail program to keep the CC list...)
On 2021-03-17 08:15:43 -0700, Andres Freund wrote:
> On Wed, Mar 17, 2021, at 07:16, Tom Lane wrote:
> > Andres Freund <and...@anarazel.de> writes:
> > > On 2021-03-16 20:50:17 -0700, Andres Freund wrote:
> > Meanwhile, I'm still trying to understand why valgrind is whining
> > about the rd_indexcxt identifier strings. AFAICS it shouldn't.
>
> I found a way around that late last night. Need to mark the context
> itself as an allocation. But I made a mess on the way to that and need
> to clean the patch up before sending it (and need to drop my
> girlfriend off first).
Unfortunately I didn't immediately find a way to do this while keeping
the MEMPOOL_CREATE/DESTROY in mcxt.c. The attached patch moves the pool
creation into the memory context implementations, "allocates" the
context itself as part of that pool, and changes the reset
implementation from MEMPOOL_DESTROY + MEMPOOL_CREATE to instead do
MEMPOOL_TRIM. That leaves the memory context itself valid (and thus
tracking ->ident etc), but marks all the other memory as freed.
This is just a first version, it probably needs more work, and
definitely a few comments...
After this, your changes, and the previously mentioned fixes, I get far
fewer false positives. Also found a crash / memory leak in pgstat.c due
to the new replication slot stats, but I'll start a separate thread.
There are a few leak warnings around guc.c that look like they might be
real, not false positives, and thus a bit concerning. Looks like several
guc check hooks don't bother to free the old *extra before allocating a
new one.
I suspect we might get better results from valgrind, not just for leaks
but also undefined value tracking, if we changed the way we represent
pools to utilize VALGRIND_MEMPOOL_METAPOOL |
VALGRIND_MEMPOOL_AUTO_FREE. E.g. aset.c would associate AllocBlock using
VALGRIND_MEMPOOL_ALLOC and then mcxt.c would use
VALGRIND_MALLOCLIKE_BLOCK for the individual chunk allocation.
https://www.valgrind.org/docs/manual/mc-manual.html#mc-manual.mempools
I played with naming the allocations underlying aset.c using
VALGRIND_CREATE_BLOCK(block, strlen(context->name), context->name).
That does produce better undefined-value warnings, but it seems that
e.g. the leak detector doen't have that information around. Nor does it
seem to be usable for use-afte-free. At least the latter likely because
I had to VALGRIND_DISCARD by that point...
Greetings,
Andres Freund
>From c7e69e4bccfc4da97b0b999399732e3dc806b67e Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Wed, 17 Mar 2021 10:46:39 -0700
Subject: [PATCH] Make memory contexts themselves more visible to valgrind.
---
src/include/utils/memdebug.h | 1 +
src/backend/utils/mmgr/aset.c | 22 ++++++++++++++++++++++
src/backend/utils/mmgr/generation.c | 8 ++++++++
src/backend/utils/mmgr/mcxt.c | 5 -----
src/backend/utils/mmgr/slab.c | 8 ++++++++
5 files changed, 39 insertions(+), 5 deletions(-)
diff --git a/src/include/utils/memdebug.h b/src/include/utils/memdebug.h
index e88b4c6e8ef..5988bff8839 100644
--- a/src/include/utils/memdebug.h
+++ b/src/include/utils/memdebug.h
@@ -23,6 +23,7 @@
#define VALGRIND_CHECK_MEM_IS_DEFINED(addr, size) do {} while (0)
#define VALGRIND_CREATE_MEMPOOL(context, redzones, zeroed) do {} while (0)
#define VALGRIND_DESTROY_MEMPOOL(context) do {} while (0)
+#define VALGRIND_MEMPOOL_TRIM(context, ptr, size) do {} while (0)
#define VALGRIND_MAKE_MEM_DEFINED(addr, size) do {} while (0)
#define VALGRIND_MAKE_MEM_NOACCESS(addr, size) do {} while (0)
#define VALGRIND_MAKE_MEM_UNDEFINED(addr, size) do {} while (0)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec6c130d0fb..9793ddf4a3d 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -433,6 +433,12 @@ AllocSetContextCreateInternal(MemoryContext parent,
{
/* Remove entry from freelist */
set = freelist->first_free;
+
+ VALGRIND_CREATE_MEMPOOL(set, 0, false);
+ VALGRIND_MEMPOOL_ALLOC(set, set, sizeof(AllocSetContext));
+ /* the contents are still valid, but valgrind can't know that */
+ VALGRIND_MAKE_MEM_DEFINED(set, sizeof(AllocSetContext));
+
freelist->first_free = (AllocSet) set->header.nextchild;
freelist->num_free--;
@@ -477,6 +483,8 @@ AllocSetContextCreateInternal(MemoryContext parent,
name)));
}
+ VALGRIND_CREATE_MEMPOOL(set, 0, false);
+ VALGRIND_MEMPOOL_ALLOC(set, set, sizeof(AllocSetContext));
/*
* Avoid writing code that can fail between here and MemoryContextCreate;
* we'd leak the header/initial block if we ereport in this stretch.
@@ -611,6 +619,8 @@ AllocSetReset(MemoryContext context)
Assert(context->mem_allocated == keepersize);
+ VALGRIND_MEMPOOL_TRIM(set, set, sizeof(AllocSetAlloc));
+
/* Reset block size allocation sequence, too */
set->nextBlockSize = set->initBlockSize;
}
@@ -652,6 +662,16 @@ AllocSetDelete(MemoryContext context)
if (!context->isReset)
MemoryContextResetOnly(context);
+ VALGRIND_DESTROY_MEMPOOL(context);
+
+ /*
+ * Still need to access the memory marked as invalid due to the
+ * DESTROY. We leave the memory accessible, as otherwise valgrind will
+ * complain about having leaked the memory underlying the cached
+ * sets...
+ */
+ VALGRIND_MAKE_MEM_DEFINED(set, sizeof(AllocSetContext));
+
/*
* If the freelist is full, just discard what's already in it. See
* comments with context_freelists[].
@@ -699,6 +719,8 @@ AllocSetDelete(MemoryContext context)
Assert(context->mem_allocated == keepersize);
+ VALGRIND_DESTROY_MEMPOOL(context);
+
/* Finally, free the context header, including the keeper block */
free(set);
}
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index 2b900347645..55d1f6a6526 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -235,6 +235,9 @@ GenerationContextCreate(MemoryContext parent,
name)));
}
+ VALGRIND_CREATE_MEMPOOL(set, 0, false);
+ VALGRIND_MEMPOOL_ALLOC(set, set, sizeof(GenerationContext));
+
/*
* Avoid writing code that can fail between here and MemoryContextCreate;
* we'd leak the header if we ereport in this stretch.
@@ -293,6 +296,8 @@ GenerationReset(MemoryContext context)
set->block = NULL;
Assert(dlist_is_empty(&set->blocks));
+
+ VALGRIND_MEMPOOL_TRIM(set, set, sizeof(GenerationContext));
}
/*
@@ -304,6 +309,9 @@ GenerationDelete(MemoryContext context)
{
/* Reset to release all the GenerationBlocks */
GenerationReset(context);
+
+ VALGRIND_DESTROY_MEMPOOL(context);
+
/* And free the context header */
free(context);
}
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 84472b9158e..27f969872bc 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -174,8 +174,6 @@ MemoryContextResetOnly(MemoryContext context)
context->methods->reset(context);
context->isReset = true;
- VALGRIND_DESTROY_MEMPOOL(context);
- VALGRIND_CREATE_MEMPOOL(context, 0, false);
}
}
@@ -245,7 +243,6 @@ MemoryContextDelete(MemoryContext context)
context->methods->delete_context(context);
- VALGRIND_DESTROY_MEMPOOL(context);
}
/*
@@ -782,8 +779,6 @@ MemoryContextCreate(MemoryContext node,
node->nextchild = NULL;
node->allowInCritSection = false;
}
-
- VALGRIND_CREATE_MEMPOOL(node, 0, false);
}
/*
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index 9213be7c956..c51ede1e906 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -237,6 +237,9 @@ SlabContextCreate(MemoryContext parent,
name)));
}
+ VALGRIND_CREATE_MEMPOOL(slab, 0, false);
+ VALGRIND_MEMPOOL_ALLOC(slab, slab, sizeof(SlabContext));
+
/*
* Avoid writing code that can fail between here and MemoryContextCreate;
* we'd leak the header if we ereport in this stretch.
@@ -315,6 +318,8 @@ SlabReset(MemoryContext context)
Assert(slab->nblocks == 0);
Assert(context->mem_allocated == 0);
+
+ VALGRIND_MEMPOOL_TRIM(slab, slab, sizeof(SlabContext));
}
/*
@@ -326,6 +331,9 @@ SlabDelete(MemoryContext context)
{
/* Reset to release all the SlabBlocks */
SlabReset(context);
+
+ VALGRIND_DESTROY_MEMPOOL(context);
+
/* And free the context header */
free(context);
}
--
2.29.2.540.g3cf59784d4
