On Tue, 3 Jan 2023 at 10:25, Tom Lane <[email protected]> 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
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
