Protecting allocator headers with Valgrind
Started by David Rowleyalmost 3 years ago5 messages
Over on [1], Tom mentioned that we might want to rethink the decision
to not protect chunk headers with Valgrind. That thread fixed a bug
that was accessing array element -1, which effectively was reading the
MemoryChunk at the start of the allocated chunk as an array element.
I wrote a patch to adjust the Valgrind macros to mark the MemoryChunks
as NOACCESS and that finds the bug reported on that thread (with the
fix for it reverted).
I didn't quite get a clear run at committing the changes during the
v16 cycle, but wondering since they're really just Valgrind macro
changes if anyone would object to doing it now?
I know there are a few people out there running sqlsmith and/or
sqlancer under Valgrind. It would be good to have this in so we could
address any new issues the attached patch might help them highlight.
Any objections?
(Copying in Tom and Richard same as original thread. Reposting for
more visibility of this change)
David
Attachments:
protect_MemoryChunks_with_Valgrind.patchapplication/octet-stream; name=protect_MemoryChunks_with_Valgrind.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 2589941ec4..0bbbf93672 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 oldchksize;
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;
}
@@ -1232,8 +1227,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;
}
@@ -1305,8 +1300,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;
}
@@ -1332,8 +1327,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;
}
@@ -1374,11 +1369,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;
@@ -1396,16 +1397,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 access to the chunk header. */
+ VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
+
return GetChunkSizeFromFreeListIdx(fidx) + ALLOC_CHUNKHDRSZ;
}
@@ -1471,7 +1483,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;
@@ -1566,8 +1581,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))
{
@@ -1617,12 +1632,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 71aad512b7..88f97ea246 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: Protecting allocator headers with Valgrind
On Tue, Apr 11, 2023 at 9:28 PM David Rowley <dgrowleyml@gmail.com> wrote:
Over on [1], Tom mentioned that we might want to rethink the decision
to not protect chunk headers with Valgrind. That thread fixed a bug
that was accessing array element -1, which effectively was reading the
MemoryChunk at the start of the allocated chunk as an array element.
Seems the link to the original thread is not pasted. Here it is.
[1]: /messages/by-id/1650235.1672694719@sss.pgh.pa.us
Thanks
Richard
Re: Protecting allocator headers with Valgrind
On Wed, 12 Apr 2023 at 01:28, David Rowley <dgrowleyml@gmail.com> wrote:
Any objections?
It seems there are none. I'll have another look at the patch tomorrow
with the aim to get it in.
(Unless someone objects to me doing that before then)
David
Re: Protecting allocator headers with Valgrind
On Wed, Apr 12, 2023 at 01:28:08AM +1200, David Rowley wrote:
Any objections?
Not objecting. I think the original Valgrind integration refrained from this
because it would have added enough Valgrind client requests to greatly slow
Valgrind runs. Valgrind reduced the cost of client requests in later years,
so this new conclusion is reasonable.
Re: Protecting allocator headers with Valgrind
On Sun, 16 Apr 2023 at 03:26, Noah Misch <noah@leadboat.com> wrote:
Not objecting. I think the original Valgrind integration refrained from this
because it would have added enough Valgrind client requests to greatly slow
Valgrind runs. Valgrind reduced the cost of client requests in later years,
so this new conclusion is reasonable.
I tested that. It's not much slowdown:
time make installcheck
Unpatched: real 79m36.458s
Patched: real 81m31.589s
I forgot to mention, I pushed the patch yesterday.
David