An oversight in ExecInitAgg for grouping sets
Started by Richard Guoabout 3 years ago10 messages
I happened to notice $subject. It happens when we build eqfunctions for
each grouping set.
/* for each grouping set */
for (int k = 0; k < phasedata->numsets; k++)
{
int length = phasedata->gset_lengths[k];
if (phasedata->eqfunctions[length - 1] != NULL)
continue;
phasedata->eqfunctions[length - 1] =
execTuplesMatchPrepare(scanDesc,
length,
aggnode->grpColIdx,
aggnode->grpOperators,
aggnode->grpCollations,
(PlanState *) aggstate);
}
If it is an empty grouping set, its length will be zero, and accessing
phasedata->eqfunctions[length - 1] is not right.
I think we can just skip building the eqfunctions for empty grouping
set.
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -3494,6 +3494,10 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
{
int length = phasedata->gset_lengths[k];+ /* skip empty grouping set */
+ if (length == 0)
+ continue;
+
if (phasedata->eqfunctions[length - 1] != NULL)
continue;Thanks
Richard
Re: An oversight in ExecInitAgg for grouping sets
On Thu, Dec 22, 2022 at 2:02 PM Richard Guo <guofenglinux@gmail.com> wrote:
I happened to notice $subject. It happens when we build eqfunctions for
each grouping set./* for each grouping set */
for (int k = 0; k < phasedata->numsets; k++)
{
int length = phasedata->gset_lengths[k];if (phasedata->eqfunctions[length - 1] != NULL)
continue;phasedata->eqfunctions[length - 1] =
execTuplesMatchPrepare(scanDesc,
length,
aggnode->grpColIdx,
aggnode->grpOperators,
aggnode->grpCollations,
(PlanState *) aggstate);
}If it is an empty grouping set, its length will be zero, and accessing
phasedata->eqfunctions[length - 1] is not right.I think we can just skip building the eqfunctions for empty grouping
set.
Attached is a trivial patch for the fix.
Thanks
Richard
Attachments:
v1-0001-Skip-building-eqfunctions-for-empty-grouping-set.patchapplication/octet-stream; name=v1-0001-Skip-building-eqfunctions-for-empty-grouping-set.patchDownload
From 6a814fc2219da84b3f80c43939fc1dbed42e12fa Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Tue, 27 Dec 2022 14:45:06 +0800
Subject: [PATCH v1] Skip building eqfunctions for empty grouping set
---
src/backend/executor/nodeAgg.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 30c9143183..b5f9f71a7b 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -3494,6 +3494,10 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
{
int length = phasedata->gset_lengths[k];
+ /* skip empty grouping set */
+ if (length == 0)
+ continue;
+
if (phasedata->eqfunctions[length - 1] != NULL)
continue;
--
2.31.0
Re: An oversight in ExecInitAgg for grouping sets
Richard Guo <guofenglinux@gmail.com> writes:
On Thu, Dec 22, 2022 at 2:02 PM Richard Guo <guofenglinux@gmail.com> wrote:
If it is an empty grouping set, its length will be zero, and accessing
phasedata->eqfunctions[length - 1] is not right.
Attached is a trivial patch for the fix.
Agreed, that's a latent bug. It's only latent because the word just
before a palloc chunk will never be zero, either in our historical
palloc code or in v16's shiny new implementation. Nonetheless it
is a horrible idea for ExecInitAgg to depend on that fact, so I
pushed your fix.
The thing that I find really distressing here is that it's been
like this for years and none of our automated testing caught it.
You'd have expected valgrind testing to do so ... but it does not,
because we've never marked that word NOACCESS. Maybe we should
rethink that? It'd require making mcxt.c do some valgrind flag
manipulations so it could access the hdrmask when appropriate.
regards, tom lane
Re: An oversight in ExecInitAgg for grouping sets
On Tue, Jan 3, 2023 at 5:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Agreed, that's a latent bug. It's only latent because the word just
before a palloc chunk will never be zero, either in our historical
palloc code or in v16's shiny new implementation. Nonetheless it
is a horrible idea for ExecInitAgg to depend on that fact, so I
pushed your fix.
Thanks for pushing it!
The thing that I find really distressing here is that it's been
like this for years and none of our automated testing caught it.
You'd have expected valgrind testing to do so ... but it does not,
because we've never marked that word NOACCESS. Maybe we should
rethink that? It'd require making mcxt.c do some valgrind flag
manipulations so it could access the hdrmask when appropriate.
Yeah, maybe we can do that. It's true that it requires some additional
work to access hdrmask, as in the new implementation the palloc'd chunk
is always prefixed by hdrmask.
BTW, I noticed a typo in the comment of memorychunk.h.
--- a/src/include/utils/memutils_memorychunk.h
+++ b/src/include/utils/memutils_memorychunk.h
@@ -5,9 +5,9 @@
* MemoryContexts may use as a header for chunks of memory they
allocate.
*
* MemoryChunk provides a lightweight header that a MemoryContext can use
to
- * store a reference back to the block the which the given chunk is
allocated
- * on and also an additional 30-bits to store another value such as the
size
- * of the allocated chunk.
+ * store a reference back to the block which the given chunk is allocated
on
+ * and also an additional 30-bits to store another value such as the size
of
+ * the allocated chunk.Thanks
Richard
Re: An oversight in ExecInitAgg for grouping sets
On Tue, 3 Jan 2023 at 10:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The thing that I find really distressing here is that it's been
like this for years and none of our automated testing caught it.
You'd have expected valgrind testing to do so ... but it does not,
because we've never marked that word NOACCESS. Maybe we should
rethink that? It'd require making mcxt.c do some valgrind flag
manipulations so it could access the hdrmask when appropriate.
Yeah, that probably could have been improved during the recent change.
Here's a patch for it.
I'm just doing a final Valgrind run on it now to check for errors.
David
Attachments:
valgrind_MemoryChunk.patchtext/plain; charset=US-ASCII; name=valgrind_MemoryChunk.patchDownload
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ef10bb1690..30151d57d6 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -188,14 +188,6 @@ typedef struct AllocBlockData
char *endptr; /* end of space in this block */
} AllocBlockData;
-/*
- * Only the "hdrmask" field should be accessed outside this module.
- * We keep the rest of an allocated chunk's header marked NOACCESS when using
- * valgrind. But note that chunk headers that are in a freelist are kept
- * accessible, for simplicity.
- */
-#define ALLOCCHUNK_PRIVATE_LEN offsetof(MemoryChunk, hdrmask)
-
/*
* AllocPointerIsValid
* True iff pointer is valid allocation pointer.
@@ -777,8 +769,8 @@ AllocSetAlloc(MemoryContext context, Size size)
VALGRIND_MAKE_MEM_NOACCESS((char *) MemoryChunkGetPointer(chunk) + size,
chunk_size - size);
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return MemoryChunkGetPointer(chunk);
}
@@ -823,8 +815,8 @@ AllocSetAlloc(MemoryContext context, Size size)
VALGRIND_MAKE_MEM_NOACCESS((char *) MemoryChunkGetPointer(chunk) + size,
GetChunkSizeFromFreeListIdx(fidx) - size);
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return MemoryChunkGetPointer(chunk);
}
@@ -989,8 +981,8 @@ AllocSetAlloc(MemoryContext context, Size size)
VALGRIND_MAKE_MEM_NOACCESS((char *) MemoryChunkGetPointer(chunk) + size,
chunk_size - size);
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return MemoryChunkGetPointer(chunk);
}
@@ -1005,8 +997,8 @@ AllocSetFree(void *pointer)
AllocSet set;
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
- /* Allow access to private part of chunk header. */
- VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
if (MemoryChunkIsExternal(chunk))
{
@@ -1117,8 +1109,8 @@ AllocSetRealloc(void *pointer, Size size)
Size oldsize;
int fidx;
- /* Allow access to private part of chunk header. */
- VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
if (MemoryChunkIsExternal(chunk))
{
@@ -1166,8 +1158,8 @@ AllocSetRealloc(void *pointer, Size size)
block = (AllocBlock) realloc(block, blksize);
if (block == NULL)
{
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return NULL;
}
@@ -1223,8 +1215,8 @@ AllocSetRealloc(void *pointer, Size size)
/* Ensure any padding bytes are marked NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS((char *) pointer + size, chksize - size);
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header . */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return pointer;
}
@@ -1296,8 +1288,8 @@ AllocSetRealloc(void *pointer, Size size)
VALGRIND_MAKE_MEM_DEFINED(pointer, size);
#endif
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return pointer;
}
@@ -1322,8 +1314,8 @@ AllocSetRealloc(void *pointer, Size size)
/* leave immediately if request was not completed */
if (newPointer == NULL)
{
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return NULL;
}
@@ -1363,11 +1355,17 @@ AllocSetGetChunkContext(void *pointer)
AllocBlock block;
AllocSet set;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
+
if (MemoryChunkIsExternal(chunk))
block = ExternalChunkGetBlock(chunk);
else
block = (AllocBlock) MemoryChunkGetBlock(chunk);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
+
Assert(AllocBlockIsValid(block));
set = block->aset;
@@ -1385,16 +1383,27 @@ AllocSetGetChunkSpace(void *pointer)
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
int fidx;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
+
if (MemoryChunkIsExternal(chunk))
{
AllocBlock block = ExternalChunkGetBlock(chunk);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
+
Assert(AllocBlockIsValid(block));
+
return block->endptr - (char *) chunk;
}
fidx = MemoryChunkGetValue(chunk);
Assert(FreeListIdxIsValid(fidx));
+
+ /* Disallow external access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
+
return GetChunkSizeFromFreeListIdx(fidx) + ALLOC_CHUNKHDRSZ;
}
@@ -1555,8 +1564,8 @@ AllocSetCheck(MemoryContext context)
Size chsize,
dsize;
- /* Allow access to private part of chunk header. */
- VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
if (MemoryChunkIsExternal(chunk))
{
@@ -1606,12 +1615,9 @@ AllocSetCheck(MemoryContext context)
elog(WARNING, "problem in alloc set %s: detected write past chunk end in block %p, chunk %p",
name, block, chunk);
- /*
- * If chunk is allocated, disallow external access to private part
- * of chunk header.
- */
+ /* if chunk is allocated, disallow access to the chunk header */
if (dsize != InvalidAllocSize)
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
blk_data += chsize;
nchunks++;
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index 93825265a1..3cc5e481c0 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -98,14 +98,6 @@ struct GenerationBlock
char *endptr; /* end of space in this block */
};
-/*
- * Only the "hdrmask" field should be accessed outside this module.
- * We keep the rest of an allocated chunk's header marked NOACCESS when using
- * valgrind. But note that freed chunk headers are kept accessible, for
- * simplicity.
- */
-#define GENERATIONCHUNK_PRIVATE_LEN offsetof(MemoryChunk, hdrmask)
-
/*
* GenerationIsValid
* True iff set is valid generation set.
@@ -407,8 +399,8 @@ GenerationAlloc(MemoryContext context, Size size)
VALGRIND_MAKE_MEM_NOACCESS((char *) MemoryChunkGetPointer(chunk) + size,
chunk_size - size);
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
return MemoryChunkGetPointer(chunk);
}
@@ -522,8 +514,8 @@ GenerationAlloc(MemoryContext context, Size size)
VALGRIND_MAKE_MEM_NOACCESS((char *) MemoryChunkGetPointer(chunk) + size,
chunk_size - size);
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
return MemoryChunkGetPointer(chunk);
}
@@ -664,8 +656,8 @@ GenerationFree(void *pointer)
#endif
}
- /* Allow access to private part of chunk header. */
- VALGRIND_MAKE_MEM_DEFINED(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Generation_CHUNKHDRSZ);
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
@@ -744,8 +736,8 @@ GenerationRealloc(void *pointer, Size size)
GenerationPointer newPointer;
Size oldsize;
- /* Allow access to private part of chunk header. */
- VALGRIND_MAKE_MEM_DEFINED(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Generation_CHUNKHDRSZ);
if (MemoryChunkIsExternal(chunk))
{
@@ -832,8 +824,8 @@ GenerationRealloc(void *pointer, Size size)
VALGRIND_MAKE_MEM_DEFINED(pointer, size);
#endif
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
return pointer;
}
@@ -844,8 +836,8 @@ GenerationRealloc(void *pointer, Size size)
/* leave immediately if request was not completed */
if (newPointer == NULL)
{
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
return NULL;
}
@@ -883,11 +875,17 @@ GenerationGetChunkContext(void *pointer)
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
GenerationBlock *block;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Generation_CHUNKHDRSZ);
+
if (MemoryChunkIsExternal(chunk))
block = ExternalChunkGetBlock(chunk);
else
block = (GenerationBlock *) MemoryChunkGetBlock(chunk);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
+
Assert(GenerationBlockIsValid(block));
return &block->context->header;
}
@@ -903,6 +901,9 @@ GenerationGetChunkSpace(void *pointer)
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
Size chunksize;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Generation_CHUNKHDRSZ);
+
if (MemoryChunkIsExternal(chunk))
{
GenerationBlock *block = ExternalChunkGetBlock(chunk);
@@ -913,6 +914,9 @@ GenerationGetChunkSpace(void *pointer)
else
chunksize = MemoryChunkGetValue(chunk);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
+
return Generation_CHUNKHDRSZ + chunksize;
}
@@ -1054,8 +1058,8 @@ GenerationCheck(MemoryContext context)
GenerationBlock *chunkblock;
Size chunksize;
- /* Allow access to private part of chunk header. */
- VALGRIND_MAKE_MEM_DEFINED(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Generation_CHUNKHDRSZ);
if (MemoryChunkIsExternal(chunk))
{
@@ -1098,12 +1102,9 @@ GenerationCheck(MemoryContext context)
else
nfree += 1;
- /*
- * If chunk is allocated, disallow external access to private part
- * of chunk header.
- */
+ /* if chunk is allocated, disallow access to the chunk header */
if (chunk->requested_size != InvalidAllocSize)
- VALGRIND_MAKE_MEM_NOACCESS(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
}
/*
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 8b6591abfb..8038a2fea4 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -190,8 +190,14 @@ GetMemoryChunkMethodID(const void *pointer)
*/
Assert(pointer == (const void *) MAXALIGN(pointer));
+ /* Allow access to the uint64 header */
+ VALGRIND_MAKE_MEM_DEFINED((char *) pointer - sizeof(uint64), sizeof(uint64));
+
header = *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+ /* Disallow access to the uint64 header */
+ VALGRIND_MAKE_MEM_NOACCESS((char *) pointer - sizeof(uint64), sizeof(uint64));
+
return (MemoryContextMethodID) (header & MEMORY_CONTEXT_METHODID_MASK);
}
@@ -204,7 +210,17 @@ GetMemoryChunkMethodID(const void *pointer)
static inline uint64
GetMemoryChunkHeader(const void *pointer)
{
- return *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+ uint64 header;
+
+ /* Allow access to the uint64 header */
+ VALGRIND_MAKE_MEM_DEFINED((char *) pointer - sizeof(uint64), sizeof(uint64));
+
+ header = *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+
+ /* Disallow access to the uint64 header */
+ VALGRIND_MAKE_MEM_NOACCESS((char *) pointer - sizeof(uint64), sizeof(uint64));
+
+ return header;
}
/*
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index 33dca0f37c..9fd00a98d0 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -630,6 +630,9 @@ SlabAlloc(MemoryContext context, Size size)
randomize_mem((char *) MemoryChunkGetPointer(chunk), size);
#endif
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Slab_CHUNKHDRSZ);
+
return MemoryChunkGetPointer(chunk);
}
@@ -646,6 +649,9 @@ SlabFree(void *pointer)
int curBlocklistIdx;
int newBlocklistIdx;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Slab_CHUNKHDRSZ);
+
/*
* For speed reasons we just Assert that the referenced block is good.
* Future field experience may show that this Assert had better become a
@@ -761,9 +767,14 @@ void *
SlabRealloc(void *pointer, Size size)
{
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
- SlabBlock *block = MemoryChunkGetBlock(chunk);
+ SlabBlock *block;
SlabContext *slab;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Slab_CHUNKHDRSZ);
+
+ block = MemoryChunkGetBlock(chunk);
+
/*
* Try to verify that we have a sane block pointer: the block header
* should reference a slab context. (We use a test-and-elog, not just
@@ -790,9 +801,18 @@ MemoryContext
SlabGetChunkContext(void *pointer)
{
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
- SlabBlock *block = MemoryChunkGetBlock(chunk);
+ SlabBlock *block;
+
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Slab_CHUNKHDRSZ);
+
+ block = MemoryChunkGetBlock(chunk);
+
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Slab_CHUNKHDRSZ);
Assert(SlabBlockIsValid(block));
+
return &block->slab->header;
}
@@ -805,9 +825,17 @@ Size
SlabGetChunkSpace(void *pointer)
{
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
- SlabBlock *block = MemoryChunkGetBlock(chunk);
+ SlabBlock *block;
SlabContext *slab;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Slab_CHUNKHDRSZ);
+
+ block = MemoryChunkGetBlock(chunk);
+
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Slab_CHUNKHDRSZ);
+
Assert(SlabBlockIsValid(block));
slab = block->slab;
@@ -1017,7 +1045,15 @@ SlabCheck(MemoryContext context)
if (!slab->isChunkFree[j])
{
MemoryChunk *chunk = SlabBlockGetChunk(slab, block, j);
- SlabBlock *chunkblock = (SlabBlock *) MemoryChunkGetBlock(chunk);
+ SlabBlock *chunkblock;
+
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Slab_CHUNKHDRSZ);
+
+ chunkblock = (SlabBlock *) MemoryChunkGetBlock(chunk);
+
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Slab_CHUNKHDRSZ);
/*
* check the chunk's blockoffset correctly points back to
Re: An oversight in ExecInitAgg for grouping sets
On Thu, Jan 5, 2023 at 6:18 AM David Rowley <dgrowleyml@gmail.com> wrote:
On Tue, 3 Jan 2023 at 10:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The thing that I find really distressing here is that it's been
like this for years and none of our automated testing caught it.
You'd have expected valgrind testing to do so ... but it does not,
because we've never marked that word NOACCESS. Maybe we should
rethink that? It'd require making mcxt.c do some valgrind flag
manipulations so it could access the hdrmask when appropriate.Yeah, that probably could have been improved during the recent change.
Here's a patch for it.
Thanks for the patch. With it Valgrind is able to catch the invalid
read discussed in the initial email of this thread.
VALGRINDERROR-BEGIN
Invalid read of size 8
at 0x4DB056: ExecInitAgg
by 0x4C486A: ExecInitNode
by 0x4B92B7: InitPlan
by 0x4B81D7: standard_ExecutorStart
by 0x4B7F1B: ExecutorStart
I reviewed this patch and have some comments.
It seems that for MemoryContextMethods in alignedalloc.c the access to
the chunk header is not wrapped by VALGRIND_MAKE_MEM_DEFINED and
VALGRIND_MAKE_MEM_NOACCESS. Should we do that?
In GenerationFree, I think the VALGRIND_MAKE_MEM_DEFINED should be moved
to the start of this function, before we call MemoryChunkIsExternal.
In SlabFree, we should call MemoryChunkGetBlock after the call of
VALGRIND_MAKE_MEM_DEFINED, just like how we do in SlabRealloc.
In AllocSetStats, we have a call of MemoryChunkGetValue in Assert. I
think we should wrap it with VALGRIND_MAKE_MEM_DEFINED and
VALGRIND_MAKE_MEM_NOACCESS.
Thanks
Richard
Re: An oversight in ExecInitAgg for grouping sets
On Thu, 5 Jan 2023 at 20:06, Richard Guo <guofenglinux@gmail.com> wrote:
I reviewed this patch and have some comments.
Thanks for looking at this. I think I've fixed all the issues you mentioned.
One extra thing I noticed was that I had to add a new
VALGRIND_MAKE_MEM_DEFINED in AllocSetAlloc when grabbing an item off
the freelist. I didn't quite manage to figure out why that's needed as
when we do AllocSetFree() we don't mark the pfree'd memory with
NOACCESS, and it also looks like AllocSetReset() sets the keeper
block's memory to NOACCESS, but that function also clears the
freelists too, so the freelist chunk is not coming from a recently
reset context.
I might need to spend a bit more time on this to see if I can figure
out why this is happening. On the other hand, maybe we should just
mark pfree'd memory as NOACCESS as that might find another class of
issues.
David
Attachments:
valgrind_memorychunk_v2.patchtext/plain; charset=US-ASCII; name=valgrind_memorychunk_v2.patchDownload
diff --git a/src/backend/utils/mmgr/alignedalloc.c b/src/backend/utils/mmgr/alignedalloc.c
index 9b975739d1..627e988852 100644
--- a/src/backend/utils/mmgr/alignedalloc.c
+++ b/src/backend/utils/mmgr/alignedalloc.c
@@ -31,6 +31,8 @@ AlignedAllocFree(void *pointer)
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
void *unaligned;
+ VALGRIND_MAKE_MEM_DEFINED(chunk, sizeof(MemoryChunk));
+
Assert(!MemoryChunkIsExternal(chunk));
/* obtain the original (unaligned) allocated pointer */
@@ -58,12 +60,17 @@ void *
AlignedAllocRealloc(void *pointer, Size size)
{
MemoryChunk *redirchunk = PointerGetMemoryChunk(pointer);
- Size alignto = MemoryChunkGetValue(redirchunk);
- void *unaligned = MemoryChunkGetBlock(redirchunk);
+ Size alignto;
+ void *unaligned;
MemoryContext ctx;
Size old_size;
void *newptr;
+ VALGRIND_MAKE_MEM_DEFINED(redirchunk, sizeof(MemoryChunk));
+
+ alignto = MemoryChunkGetValue(redirchunk);
+ unaligned = MemoryChunkGetBlock(redirchunk);
+
/* sanity check this is a power of 2 value */
Assert((alignto & (alignto - 1)) == 0);
@@ -110,11 +117,18 @@ AlignedAllocRealloc(void *pointer, Size size)
MemoryContext
AlignedAllocGetChunkContext(void *pointer)
{
- MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
+ MemoryChunk *redirchunk = PointerGetMemoryChunk(pointer);
+ MemoryContext cxt;
- Assert(!MemoryChunkIsExternal(chunk));
+ VALGRIND_MAKE_MEM_DEFINED(redirchunk, sizeof(MemoryChunk));
- return GetMemoryChunkContext(MemoryChunkGetBlock(chunk));
+ Assert(!MemoryChunkIsExternal(redirchunk));
+
+ cxt = GetMemoryChunkContext(MemoryChunkGetBlock(redirchunk));
+
+ VALGRIND_MAKE_MEM_NOACCESS(redirchunk, sizeof(MemoryChunk));
+
+ return cxt;
}
/*
@@ -126,7 +140,15 @@ Size
AlignedAllocGetChunkSpace(void *pointer)
{
MemoryChunk *redirchunk = PointerGetMemoryChunk(pointer);
- void *unaligned = MemoryChunkGetBlock(redirchunk);
+ void *unaligned;
+ Size space;
+
+ VALGRIND_MAKE_MEM_DEFINED(redirchunk, sizeof(MemoryChunk));
+
+ unaligned = MemoryChunkGetBlock(redirchunk);
+ space = GetMemoryChunkSpace(unaligned);
+
+ VALGRIND_MAKE_MEM_NOACCESS(redirchunk, sizeof(MemoryChunk));
- return GetMemoryChunkSpace(unaligned);
+ return space;
}
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 740729b5d0..2612a92e33 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -188,14 +188,6 @@ typedef struct AllocBlockData
char *endptr; /* end of space in this block */
} AllocBlockData;
-/*
- * Only the "hdrmask" field should be accessed outside this module.
- * We keep the rest of an allocated chunk's header marked NOACCESS when using
- * valgrind. But note that chunk headers that are in a freelist are kept
- * accessible, for simplicity.
- */
-#define ALLOCCHUNK_PRIVATE_LEN offsetof(MemoryChunk, hdrmask)
-
/*
* AllocPointerIsValid
* True iff pointer is valid allocation pointer.
@@ -777,8 +769,8 @@ AllocSetAlloc(MemoryContext context, Size size)
VALGRIND_MAKE_MEM_NOACCESS((char *) MemoryChunkGetPointer(chunk) + size,
chunk_size - size);
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return MemoryChunkGetPointer(chunk);
}
@@ -801,6 +793,9 @@ AllocSetAlloc(MemoryContext context, Size size)
{
AllocFreeListLink *link = GetFreeListLink(chunk);
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
+
Assert(fidx == MemoryChunkGetValue(chunk));
/* pop this chunk off the freelist */
@@ -823,8 +818,8 @@ AllocSetAlloc(MemoryContext context, Size size)
VALGRIND_MAKE_MEM_NOACCESS((char *) MemoryChunkGetPointer(chunk) + size,
GetChunkSizeFromFreeListIdx(fidx) - size);
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return MemoryChunkGetPointer(chunk);
}
@@ -989,8 +984,8 @@ AllocSetAlloc(MemoryContext context, Size size)
VALGRIND_MAKE_MEM_NOACCESS((char *) MemoryChunkGetPointer(chunk) + size,
chunk_size - size);
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return MemoryChunkGetPointer(chunk);
}
@@ -1005,8 +1000,8 @@ AllocSetFree(void *pointer)
AllocSet set;
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
- /* Allow access to private part of chunk header. */
- VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
if (MemoryChunkIsExternal(chunk))
{
@@ -1115,8 +1110,8 @@ AllocSetRealloc(void *pointer, Size size)
Size oldsize;
int fidx;
- /* Allow access to private part of chunk header. */
- VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
if (MemoryChunkIsExternal(chunk))
{
@@ -1164,8 +1159,8 @@ AllocSetRealloc(void *pointer, Size size)
block = (AllocBlock) realloc(block, blksize);
if (block == NULL)
{
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return NULL;
}
@@ -1221,8 +1216,8 @@ AllocSetRealloc(void *pointer, Size size)
/* Ensure any padding bytes are marked NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS((char *) pointer + size, chksize - size);
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header . */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return pointer;
}
@@ -1294,8 +1289,8 @@ AllocSetRealloc(void *pointer, Size size)
VALGRIND_MAKE_MEM_DEFINED(pointer, size);
#endif
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return pointer;
}
@@ -1320,8 +1315,8 @@ AllocSetRealloc(void *pointer, Size size)
/* leave immediately if request was not completed */
if (newPointer == NULL)
{
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
return NULL;
}
@@ -1361,11 +1356,17 @@ AllocSetGetChunkContext(void *pointer)
AllocBlock block;
AllocSet set;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
+
if (MemoryChunkIsExternal(chunk))
block = ExternalChunkGetBlock(chunk);
else
block = (AllocBlock) MemoryChunkGetBlock(chunk);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
+
Assert(AllocBlockIsValid(block));
set = block->aset;
@@ -1383,16 +1384,27 @@ AllocSetGetChunkSpace(void *pointer)
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
int fidx;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
+
if (MemoryChunkIsExternal(chunk))
{
AllocBlock block = ExternalChunkGetBlock(chunk);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
+
Assert(AllocBlockIsValid(block));
+
return block->endptr - (char *) chunk;
}
fidx = MemoryChunkGetValue(chunk);
Assert(FreeListIdxIsValid(fidx));
+
+ /* Disallow external access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
+
return GetChunkSizeFromFreeListIdx(fidx) + ALLOC_CHUNKHDRSZ;
}
@@ -1458,7 +1470,10 @@ AllocSetStats(MemoryContext context,
{
AllocFreeListLink *link = GetFreeListLink(chunk);
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
Assert(MemoryChunkGetValue(chunk) == fidx);
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
freechunks++;
freespace += chksz + ALLOC_CHUNKHDRSZ;
@@ -1553,8 +1568,8 @@ AllocSetCheck(MemoryContext context)
Size chsize,
dsize;
- /* Allow access to private part of chunk header. */
- VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
if (MemoryChunkIsExternal(chunk))
{
@@ -1604,12 +1619,9 @@ AllocSetCheck(MemoryContext context)
elog(WARNING, "problem in alloc set %s: detected write past chunk end in block %p, chunk %p",
name, block, chunk);
- /*
- * If chunk is allocated, disallow external access to private part
- * of chunk header.
- */
+ /* if chunk is allocated, disallow access to the chunk header */
if (dsize != InvalidAllocSize)
- VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
blk_data += chsize;
nchunks++;
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index ebcb61e9b6..ae48cdbaa9 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -98,14 +98,6 @@ struct GenerationBlock
char *endptr; /* end of space in this block */
};
-/*
- * Only the "hdrmask" field should be accessed outside this module.
- * We keep the rest of an allocated chunk's header marked NOACCESS when using
- * valgrind. But note that freed chunk headers are kept accessible, for
- * simplicity.
- */
-#define GENERATIONCHUNK_PRIVATE_LEN offsetof(MemoryChunk, hdrmask)
-
/*
* GenerationIsValid
* True iff set is valid generation set.
@@ -407,8 +399,8 @@ GenerationAlloc(MemoryContext context, Size size)
VALGRIND_MAKE_MEM_NOACCESS((char *) MemoryChunkGetPointer(chunk) + size,
chunk_size - size);
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
return MemoryChunkGetPointer(chunk);
}
@@ -522,8 +514,8 @@ GenerationAlloc(MemoryContext context, Size size)
VALGRIND_MAKE_MEM_NOACCESS((char *) MemoryChunkGetPointer(chunk) + size,
chunk_size - size);
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
return MemoryChunkGetPointer(chunk);
}
@@ -634,6 +626,9 @@ GenerationFree(void *pointer)
Size chunksize;
#endif
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Generation_CHUNKHDRSZ);
+
if (MemoryChunkIsExternal(chunk))
{
block = ExternalChunkGetBlock(chunk);
@@ -667,9 +662,6 @@ GenerationFree(void *pointer)
#endif
}
- /* Allow access to private part of chunk header. */
- VALGRIND_MAKE_MEM_DEFINED(chunk, GENERATIONCHUNK_PRIVATE_LEN);
-
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
Assert(chunk->requested_size < chunksize);
@@ -747,8 +739,8 @@ GenerationRealloc(void *pointer, Size size)
GenerationPointer newPointer;
Size oldsize;
- /* Allow access to private part of chunk header. */
- VALGRIND_MAKE_MEM_DEFINED(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Generation_CHUNKHDRSZ);
if (MemoryChunkIsExternal(chunk))
{
@@ -835,8 +827,8 @@ GenerationRealloc(void *pointer, Size size)
VALGRIND_MAKE_MEM_DEFINED(pointer, size);
#endif
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
return pointer;
}
@@ -847,8 +839,8 @@ GenerationRealloc(void *pointer, Size size)
/* leave immediately if request was not completed */
if (newPointer == NULL)
{
- /* Disallow external access to private part of chunk header. */
- VALGRIND_MAKE_MEM_NOACCESS(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
return NULL;
}
@@ -886,11 +878,17 @@ GenerationGetChunkContext(void *pointer)
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
GenerationBlock *block;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Generation_CHUNKHDRSZ);
+
if (MemoryChunkIsExternal(chunk))
block = ExternalChunkGetBlock(chunk);
else
block = (GenerationBlock *) MemoryChunkGetBlock(chunk);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
+
Assert(GenerationBlockIsValid(block));
return &block->context->header;
}
@@ -906,6 +904,9 @@ GenerationGetChunkSpace(void *pointer)
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
Size chunksize;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Generation_CHUNKHDRSZ);
+
if (MemoryChunkIsExternal(chunk))
{
GenerationBlock *block = ExternalChunkGetBlock(chunk);
@@ -916,6 +917,9 @@ GenerationGetChunkSpace(void *pointer)
else
chunksize = MemoryChunkGetValue(chunk);
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
+
return Generation_CHUNKHDRSZ + chunksize;
}
@@ -1057,8 +1061,8 @@ GenerationCheck(MemoryContext context)
GenerationBlock *chunkblock;
Size chunksize;
- /* Allow access to private part of chunk header. */
- VALGRIND_MAKE_MEM_DEFINED(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Generation_CHUNKHDRSZ);
if (MemoryChunkIsExternal(chunk))
{
@@ -1101,12 +1105,9 @@ GenerationCheck(MemoryContext context)
else
nfree += 1;
- /*
- * If chunk is allocated, disallow external access to private part
- * of chunk header.
- */
+ /* if chunk is allocated, disallow access to the chunk header */
if (chunk->requested_size != InvalidAllocSize)
- VALGRIND_MAKE_MEM_NOACCESS(chunk, GENERATIONCHUNK_PRIVATE_LEN);
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Generation_CHUNKHDRSZ);
}
/*
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 8b6591abfb..581fc7a141 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -190,8 +190,14 @@ GetMemoryChunkMethodID(const void *pointer)
*/
Assert(pointer == (const void *) MAXALIGN(pointer));
+ /* Allow access to the uint64 header */
+ VALGRIND_MAKE_MEM_DEFINED((char *) pointer - sizeof(uint64), sizeof(uint64));
+
header = *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+ /* Disallow access to the uint64 header */
+ VALGRIND_MAKE_MEM_NOACCESS((char *) pointer - sizeof(uint64), sizeof(uint64));
+
return (MemoryContextMethodID) (header & MEMORY_CONTEXT_METHODID_MASK);
}
@@ -204,7 +210,17 @@ GetMemoryChunkMethodID(const void *pointer)
static inline uint64
GetMemoryChunkHeader(const void *pointer)
{
- return *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+ uint64 header;
+
+ /* Allow access to the uint64 header */
+ VALGRIND_MAKE_MEM_DEFINED((char *) pointer - sizeof(uint64), sizeof(uint64));
+
+ header = *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+
+ /* Disallow access to the uint64 header */
+ VALGRIND_MAKE_MEM_NOACCESS((char *) pointer - sizeof(uint64), sizeof(uint64));
+
+ return header;
}
/*
@@ -1404,6 +1420,10 @@ MemoryContextAllocAligned(MemoryContext context,
/* Mark the bytes before the redirection header as noaccess */
VALGRIND_MAKE_MEM_NOACCESS(unaligned,
(char *) alignedchunk - (char *) unaligned);
+
+ /* Disallow access to the redirection chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(alignedchunk, sizeof(MemoryChunk));
+
return aligned;
}
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index 33dca0f37c..718dd2ba03 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -630,6 +630,9 @@ SlabAlloc(MemoryContext context, Size size)
randomize_mem((char *) MemoryChunkGetPointer(chunk), size);
#endif
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Slab_CHUNKHDRSZ);
+
return MemoryChunkGetPointer(chunk);
}
@@ -641,11 +644,16 @@ void
SlabFree(void *pointer)
{
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
- SlabBlock *block = MemoryChunkGetBlock(chunk);
+ SlabBlock *block;
SlabContext *slab;
int curBlocklistIdx;
int newBlocklistIdx;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Slab_CHUNKHDRSZ);
+
+ block = MemoryChunkGetBlock(chunk);
+
/*
* For speed reasons we just Assert that the referenced block is good.
* Future field experience may show that this Assert had better become a
@@ -761,9 +769,17 @@ void *
SlabRealloc(void *pointer, Size size)
{
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
- SlabBlock *block = MemoryChunkGetBlock(chunk);
+ SlabBlock *block;
SlabContext *slab;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Slab_CHUNKHDRSZ);
+
+ block = MemoryChunkGetBlock(chunk);
+
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Slab_CHUNKHDRSZ);
+
/*
* Try to verify that we have a sane block pointer: the block header
* should reference a slab context. (We use a test-and-elog, not just
@@ -790,9 +806,18 @@ MemoryContext
SlabGetChunkContext(void *pointer)
{
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
- SlabBlock *block = MemoryChunkGetBlock(chunk);
+ SlabBlock *block;
+
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Slab_CHUNKHDRSZ);
+
+ block = MemoryChunkGetBlock(chunk);
+
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Slab_CHUNKHDRSZ);
Assert(SlabBlockIsValid(block));
+
return &block->slab->header;
}
@@ -805,9 +830,17 @@ Size
SlabGetChunkSpace(void *pointer)
{
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
- SlabBlock *block = MemoryChunkGetBlock(chunk);
+ SlabBlock *block;
SlabContext *slab;
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Slab_CHUNKHDRSZ);
+
+ block = MemoryChunkGetBlock(chunk);
+
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Slab_CHUNKHDRSZ);
+
Assert(SlabBlockIsValid(block));
slab = block->slab;
@@ -1017,7 +1050,15 @@ SlabCheck(MemoryContext context)
if (!slab->isChunkFree[j])
{
MemoryChunk *chunk = SlabBlockGetChunk(slab, block, j);
- SlabBlock *chunkblock = (SlabBlock *) MemoryChunkGetBlock(chunk);
+ SlabBlock *chunkblock;
+
+ /* Allow access to the chunk header. */
+ VALGRIND_MAKE_MEM_DEFINED(chunk, Slab_CHUNKHDRSZ);
+
+ chunkblock = (SlabBlock *) MemoryChunkGetBlock(chunk);
+
+ /* Disallow access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, Slab_CHUNKHDRSZ);
/*
* check the chunk's blockoffset correctly points back to
Re: An oversight in ExecInitAgg for grouping sets
On Mon, Jan 9, 2023 at 5:21 PM David Rowley <dgrowleyml@gmail.com> wrote:
On Thu, 5 Jan 2023 at 20:06, Richard Guo <guofenglinux@gmail.com> wrote:
I reviewed this patch and have some comments.
Thanks for looking at this. I think I've fixed all the issues you
mentioned.One extra thing I noticed was that I had to add a new
VALGRIND_MAKE_MEM_DEFINED in AllocSetAlloc when grabbing an item off
the freelist. I didn't quite manage to figure out why that's needed as
when we do AllocSetFree() we don't mark the pfree'd memory with
NOACCESS, and it also looks like AllocSetReset() sets the keeper
block's memory to NOACCESS, but that function also clears the
freelists too, so the freelist chunk is not coming from a recently
reset context.I might need to spend a bit more time on this to see if I can figure
out why this is happening. On the other hand, maybe we should just
mark pfree'd memory as NOACCESS as that might find another class of
issues.
It occurred to me that this hasn't been applied. Should we add it to
the CF to not lose track of it?
Thanks
Richard
Re: An oversight in ExecInitAgg for grouping sets
On Mon, 20 Mar 2023 at 19:18, Richard Guo <guofenglinux@gmail.com> wrote:
It occurred to me that this hasn't been applied. Should we add it to
the CF to not lose track of it?
I have a git branch with it. That'll work for me personally as a
reminder to come back to it during the v17 cycle.
David
Re: An oversight in ExecInitAgg for grouping sets
On Mon, 9 Jan 2023 at 22:21, David Rowley <dgrowleyml@gmail.com> wrote:
One extra thing I noticed was that I had to add a new
VALGRIND_MAKE_MEM_DEFINED in AllocSetAlloc when grabbing an item off
the freelist. I didn't quite manage to figure out why that's needed as
when we do AllocSetFree() we don't mark the pfree'd memory with
NOACCESS, and it also looks like AllocSetReset() sets the keeper
block's memory to NOACCESS, but that function also clears the
freelists too, so the freelist chunk is not coming from a recently
reset context.
It seems I didn't look hard enough for NOACCESS marking. It's in
wipe_mem(). So that explains why the VALGRIND_MAKE_MEM_DEFINED is
required in AllocSetAlloc.
Since this patch really only touches Valgrind macros, I don't really
feel like there's a good reason we can't still do this for v16, but
I'll start another thread to increase visibility to see if anyone else
thinks differently about that.
David