Valgrind's Memcheck tool[1] is handy for finding bugs, but our use of a custom
allocator limits its ability to detect problems in unmodified PostgreSQL.
During the 9.1 beta cycle, I found some bugs[2] with a rough patch adding
instrumentation to aset.c and mcxt.c such that Memcheck understood our
allocator. I've passed that patch around to a few people over time, and I've
now removed the roughness such that it's ready for upstream. In hopes of
making things clearer in the commit history, I've split out a preliminary
refactoring patch from the main patch and attached each separately.
Besides the aset.c/mcxt.c instrumentation, this patch adds explicit checks for
undefined memory to PageAddItem() and printtup(); this has caught C-language
functions that fabricate a Datum without initializing all bits. The inclusion
of all this is controlled by a pg_config_manual.h setting. The patch also
adds a "suppression file" that directs Valgrind to silences nominal errors we
don't plan to fix.
To smoke-test the instrumentation, I used "make installcheck" runs on x86_64
GNU/Linux and ppc64 GNU/Linux. This turned up various new and newly-detected
memory bugs, which I will discuss in a separate thread. With those fixed,
"make installcheck" has a clean report (in my one particular configuration).
I designed the suppression file to work across platforms; I specifically
anticipated eventual use on x86_64 Darwin and x86_64 FreeBSD. Valgrind 3.8.1
quickly crashed when running PostgreSQL on Darwin; I did not dig further.
Since aset.c and mcxt.c contain the hottest code paths in the backend, I
verified that a !USE_VALGRIND, non-assert build produces the same code before
and after the patch. Testing that revealed the need to move up the
AllocSizeIsValid() check in repalloc(), though I don't understand why GCC
reacted that way.
Peter Geoghegan and Korry Douglas provided valuable feedback on earlier
versions of this code.
Possible future directions:
- Test "make installcheck-world". When I last did this in past years, contrib
did
trigger some errors.
- Test recovery, such as by running a streaming replica under Memcheck while
the primary runs "make installcheck-world".
- Test newer compilers and higher optimization levels. I used GCC 4.2 at -O1.
A brief look at -O2 results showed a new error that I have not studied. GCC
4.8 at -O3 might show still more due to increasingly-aggressive assumptions.
- A buildfarm member running its installcheck steps this way.
- Memcheck has support for detecting leaks. I have not explored that side at
all, always passing --leak-check=no. We could add support for freeing
"everything" at process exit, thereby making the leak detection meaningful.
Brief notes for folks reproducing my approach: I typically start the
Memcheck-hosted postgres like this:
valgrind --leak-check=no --gen-suppressions=all \
--suppressions=src/tools/valgrind.supp --time-stamp=yes \
--log-file=$HOME/pg-valgrind/%p.log postgres
If that detected an error on which I desired more detail, I would rerun a
smaller test case with "--track-origins=yes --read-var-info=yes". That slows
things noticeably but gives more-specific messaging. When even that left the
situation unclear, I would temporarily hack allocChunkLimit so every palloc()
turned into a malloc().
I strongly advise installing the latest-available Valgrind, particularly
because recent releases suffer far less of a performance drop processing the
instrumentation added by this patch. A "make installcheck" run takes 273
minutes under Vaglrind 3.6.0 but just 27 minutes under Valgrind 3.8.1.
Thanks,
nm
[1] http://valgrind.org/docs/manual/mc-manual.html
[2]
http://www.postgresql.org/message-id/[email protected]
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 6a111c7..de64377 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -308,6 +308,37 @@ AllocSetFreeIndex(Size size)
return idx;
}
+#ifdef CLOBBER_FREED_MEMORY
+
+/* Wipe freed memory for debugging purposes */
+static void
+wipe_mem(void *ptr, size_t size)
+{
+ memset(ptr, 0x7F, size);
+}
+#endif
+
+#ifdef MEMORY_CONTEXT_CHECKING
+static void
+set_sentinel(void *base, Size offset)
+{
+ char *ptr = (char *) base + offset;
+
+ *ptr = 0x7E;
+}
+
+static bool
+sentinel_ok(const void *base, Size offset)
+{
+ const char *ptr = (const char *) base + offset;
+ bool ret;
+
+ ret = *ptr == 0x7E;
+
+ return ret;
+}
+#endif
+
#ifdef RANDOMIZE_ALLOCATED_MEMORY
/*
@@ -492,8 +523,7 @@ AllocSetReset(MemoryContext context)
char *datastart = ((char *) block) +
ALLOC_BLOCKHDRSZ;
#ifdef CLOBBER_FREED_MEMORY
- /* Wipe freed memory for debugging purposes */
- memset(datastart, 0x7F, block->freeptr - datastart);
+ wipe_mem(datastart, block->freeptr - datastart);
#endif
block->freeptr = datastart;
block->next = NULL;
@@ -502,8 +532,7 @@ AllocSetReset(MemoryContext context)
{
/* Normal case, release the block */
#ifdef CLOBBER_FREED_MEMORY
- /* Wipe freed memory for debugging purposes */
- memset(block, 0x7F, block->freeptr - ((char *) block));
+ wipe_mem(block, block->freeptr - ((char *) block));
#endif
free(block);
}
@@ -545,8 +574,7 @@ AllocSetDelete(MemoryContext context)
AllocBlock next = block->next;
#ifdef CLOBBER_FREED_MEMORY
- /* Wipe freed memory for debugging purposes */
- memset(block, 0x7F, block->freeptr - ((char *) block));
+ wipe_mem(block, block->freeptr - ((char *) block));
#endif
free(block);
block = next;
@@ -598,7 +626,7 @@ AllocSetAlloc(MemoryContext context, Size size)
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
if (size < chunk_size)
- ((char *) AllocChunkGetPointer(chunk))[size] = 0x7E;
+ set_sentinel(AllocChunkGetPointer(chunk), size);
#endif
#ifdef RANDOMIZE_ALLOCATED_MEMORY
/* fill the allocated space with junk */
@@ -644,7 +672,7 @@ AllocSetAlloc(MemoryContext context, Size size)
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
if (size < chunk->size)
- ((char *) AllocChunkGetPointer(chunk))[size] = 0x7E;
+ set_sentinel(AllocChunkGetPointer(chunk), size);
#endif
#ifdef RANDOMIZE_ALLOCATED_MEMORY
/* fill the allocated space with junk */
@@ -801,7 +829,7 @@ AllocSetAlloc(MemoryContext context, Size size)
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
if (size < chunk->size)
- ((char *) AllocChunkGetPointer(chunk))[size] = 0x7E;
+ set_sentinel(AllocChunkGetPointer(chunk), size);
#endif
#ifdef RANDOMIZE_ALLOCATED_MEMORY
/* fill the allocated space with junk */
@@ -827,7 +855,7 @@ AllocSetFree(MemoryContext context, void *pointer)
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
if (chunk->requested_size < chunk->size)
- if (((char *) pointer)[chunk->requested_size] != 0x7E)
+ if (!sentinel_ok(pointer, chunk->requested_size))
elog(WARNING, "detected write past chunk end in %s %p",
set->header.name, chunk);
#endif
@@ -860,8 +888,7 @@ AllocSetFree(MemoryContext context, void *pointer)
else
prevblock->next = block->next;
#ifdef CLOBBER_FREED_MEMORY
- /* Wipe freed memory for debugging purposes */
- memset(block, 0x7F, block->freeptr - ((char *) block));
+ wipe_mem(block, block->freeptr - ((char *) block));
#endif
free(block);
}
@@ -873,8 +900,7 @@ AllocSetFree(MemoryContext context, void *pointer)
chunk->aset = (void *) set->freelist[fidx];
#ifdef CLOBBER_FREED_MEMORY
- /* Wipe freed memory for debugging purposes */
- memset(pointer, 0x7F, chunk->size);
+ wipe_mem(pointer, chunk->size);
#endif
#ifdef MEMORY_CONTEXT_CHECKING
@@ -901,7 +927,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size
size)
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
if (chunk->requested_size < oldsize)
- if (((char *) pointer)[chunk->requested_size] != 0x7E)
+ if (!sentinel_ok(pointer, chunk->requested_size))
elog(WARNING, "detected write past chunk end in %s %p",
set->header.name, chunk);
#endif
@@ -924,7 +950,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size
size)
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
if (size < oldsize)
- ((char *) pointer)[size] = 0x7E;
+ set_sentinel(pointer, size);
#endif
return pointer;
}
@@ -987,7 +1013,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size
size)
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
if (size < chunk->size)
- ((char *) AllocChunkGetPointer(chunk))[size] = 0x7E;
+ set_sentinel(AllocChunkGetPointer(chunk), size);
#endif
return AllocChunkGetPointer(chunk);
@@ -1136,11 +1162,9 @@ AllocSetCheck(MemoryContext context)
AllocChunk chunk = (AllocChunk) bpoz;
Size chsize,
dsize;
- char *chdata_end;
chsize = chunk->size; /* aligned chunk size */
dsize = chunk->requested_size; /* real data */
- chdata_end = ((char *) chunk) + (ALLOC_CHUNKHDRSZ +
dsize);
/*
* Check chunk size
@@ -1170,7 +1194,8 @@ AllocSetCheck(MemoryContext context)
/*
* Check for overwrite of "unallocated" space in chunk
*/
- if (dsize > 0 && dsize < chsize && *chdata_end != 0x7E)
+ if (dsize > 0 && dsize < chsize &&
+ !sentinel_ok(chunk, ALLOC_CHUNKHDRSZ + dsize))
elog(WARNING, "problem in alloc set %s:
detected write past chunk end in block %p, chunk %p",
name, block, chunk);
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 8dd3cf4..c72e347 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -569,6 +569,8 @@ MemoryContextCreate(NodeTag tag, Size size,
void *
MemoryContextAlloc(MemoryContext context, Size size)
{
+ void *ret;
+
AssertArg(MemoryContextIsValid(context));
if (!AllocSizeIsValid(size))
@@ -577,7 +579,9 @@ MemoryContextAlloc(MemoryContext context, Size size)
context->isReset = false;
- return (*context->methods->alloc) (context, size);
+ ret = (*context->methods->alloc) (context, size);
+
+ return ret;
}
/*
@@ -638,6 +642,8 @@ void *
palloc(Size size)
{
/* duplicates MemoryContextAlloc to avoid increased overhead */
+ void *ret;
+
AssertArg(MemoryContextIsValid(CurrentMemoryContext));
if (!AllocSizeIsValid(size))
@@ -646,7 +652,9 @@ palloc(Size size)
CurrentMemoryContext->isReset = false;
- return (*CurrentMemoryContext->methods->alloc) (CurrentMemoryContext,
size);
+ ret = (*CurrentMemoryContext->methods->alloc) (CurrentMemoryContext,
size);
+
+ return ret;
}
void *
@@ -677,7 +685,7 @@ palloc0(Size size)
void
pfree(void *pointer)
{
- StandardChunkHeader *header;
+ MemoryContext context;
/*
* Try to detect bogus pointers handed to us, poorly though we can.
@@ -690,12 +698,12 @@ pfree(void *pointer)
/*
* OK, it's probably safe to look at the chunk header.
*/
- header = (StandardChunkHeader *)
- ((char *) pointer - STANDARDCHUNKHEADERSIZE);
+ context = ((StandardChunkHeader *)
+ ((char *) pointer -
STANDARDCHUNKHEADERSIZE))->context;
- AssertArg(MemoryContextIsValid(header->context));
+ AssertArg(MemoryContextIsValid(context));
- (*header->context->methods->free_p) (header->context, pointer);
+ (*context->methods->free_p) (context, pointer);
}
/*
@@ -705,7 +713,12 @@ pfree(void *pointer)
void *
repalloc(void *pointer, Size size)
{
- StandardChunkHeader *header;
+ MemoryContext context;
+ void *ret;
+
+ if (!AllocSizeIsValid(size))
+ elog(ERROR, "invalid memory alloc request size %lu",
+ (unsigned long) size);
/*
* Try to detect bogus pointers handed to us, poorly though we can.
@@ -718,20 +731,17 @@ repalloc(void *pointer, Size size)
/*
* OK, it's probably safe to look at the chunk header.
*/
- header = (StandardChunkHeader *)
- ((char *) pointer - STANDARDCHUNKHEADERSIZE);
+ context = ((StandardChunkHeader *)
+ ((char *) pointer -
STANDARDCHUNKHEADERSIZE))->context;
- AssertArg(MemoryContextIsValid(header->context));
-
- if (!AllocSizeIsValid(size))
- elog(ERROR, "invalid memory alloc request size %lu",
- (unsigned long) size);
+ AssertArg(MemoryContextIsValid(context));
/* isReset must be false already */
- Assert(!header->context->isReset);
+ Assert(!context->isReset);
+
+ ret = (*context->methods->realloc) (context, pointer, size);
- return (*header->context->methods->realloc) (header->context,
-
pointer, size);
+ return ret;
}
/*
*** a/src/backend/access/common/printtup.c
--- b/src/backend/access/common/printtup.c
***************
*** 20,25 ****
--- 20,26 ----
#include "libpq/pqformat.h"
#include "tcop/pquery.h"
#include "utils/lsyscache.h"
+ #include "utils/memdebug.h"
static void printtup_startup(DestReceiver *self, int operation,
***************
*** 324,332 **** printtup(TupleTableSlot *slot, DestReceiver *self)
--- 325,350 ----
/*
* If we have a toasted datum, forcibly detoast it here to avoid
* memory leakage inside the type's output routine.
+ *
+ * Here we catch undefined bytes in tuples that are returned to
the
+ * client without hitting disk; see comments at the related
check in
+ * PageAddItem(). Whether to test before or after detoast is
somewhat
+ * arbitrary, as is whether to test external/compressed data at
all.
+ * Undefined bytes in the pre-toast datum will have triggered
Valgrind
+ * errors in the compressor or toaster; any error detected here
for
+ * such datums would indicate an (unlikely) bug in a
type-independent
+ * facility. Therefore, this test is most useful for
uncompressed,
+ * non-external datums.
+ *
+ * We don't presently bother checking non-varlena datums for
undefined
+ * data. PageAddItem() does check them.
*/
if (thisState->typisvarlena)
+ {
+ VALGRIND_CHECK_MEM_IS_DEFINED(origattr,
VARSIZE_ANY(origattr));
+
attr = PointerGetDatum(PG_DETOAST_DATUM(origattr));
+ }
else
attr = origattr;
*** a/src/backend/storage/page/bufpage.c
--- b/src/backend/storage/page/bufpage.c
***************
*** 17,22 ****
--- 17,23 ----
#include "access/htup_details.h"
#include "access/xlog.h"
#include "storage/checksum.h"
+ #include "utils/memdebug.h"
bool ignore_checksum_failure = false;
***************
*** 298,303 **** PageAddItem(Page page,
--- 299,318 ----
/* set the item pointer */
ItemIdSetNormal(itemId, upper, size);
+ /*
+ * Items normally contain no uninitialized bytes. Core bufpage
consumers
+ * conform, but this is not a necessary coding rule; a new index AM
could
+ * opt to depart from it. However, data type input functions and other
+ * C-language functions that synthesize datums should initialize all
+ * bytes; datumIsEqual() relies on this. Testing here, along with the
+ * similar check in printtup(), helps to catch such mistakes.
+ *
+ * Values of the "name" type retrieved via index-only scans may contain
+ * uninitialized bytes; see comment in btrescan(). Valgrind will report
+ * this as an error, but it is safe to ignore.
+ */
+ VALGRIND_CHECK_MEM_IS_DEFINED(item, size);
+
/* copy the item's data onto the page */
memcpy((char *) page + upper, item, size);
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
***************
*** 69,74 ****
--- 69,75 ----
#include "tcop/tcopprot.h"
#include "tcop/utility.h"
#include "utils/lsyscache.h"
+ #include "utils/memdebug.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
#include "utils/snapmgr.h"
***************
*** 846,851 **** exec_simple_query(const char *query_string)
--- 847,856 ----
TRACE_POSTGRESQL_QUERY_START(query_string);
+ #ifdef USE_VALGRIND
+ VALGRIND_PRINTF("statement: %s\n", query_string);
+ #endif
+
/*
* We use save_log_statement_stats so ShowUsage doesn't report incorrect
* results because ResetUsage wasn't called.
*** a/src/backend/utils/mmgr/aset.c
--- b/src/backend/utils/mmgr/aset.c
***************
*** 59,69 ****
--- 59,92 ----
* the requested space whenever the request is less than the actual chunk
* size, and verifies that the byte is undamaged when the chunk is freed.
*
+ *
+ * About USE_VALGRIND and Valgrind client requests:
+ *
+ * Valgrind provides "client request" macros that exchange information with
+ * the host Valgrind (if any). Under !USE_VALGRIND, memdebug.h stubs out
+ * currently-used macros.
+ *
+ * When running under Valgrind, we want a NOACCESS memory region both
before
+ * and after the allocation. The chunk header is tempting as the preceding
+ * region, but mcxt.c expects to able to examine the standard chunk header
+ * fields. Therefore, we use, when available, the requested_size field and
+ * any subsequent padding. requested_size is made NOACCESS before
returning
+ * a chunk pointer to a caller. However, to reduce client request traffic,
+ * it is kept DEFINED in chunks on the free list.
+ *
+ * The rounded-up capacity of the chunk usually acts as a post-allocation
+ * NOACCESS region. If the request consumes precisely the entire chunk,
+ * there is no such region; another chunk header may immediately follow.
In
+ * that case, Valgrind will not detect access beyond the end of the chunk.
+ *
+ * See also the cooperating Valgrind client requests in mcxt.c.
+ *
*-------------------------------------------------------------------------
*/
#include "postgres.h"
+ #include "utils/memdebug.h"
#include "utils/memutils.h"
/* Define this to detail debug alloc information */
***************
*** 116,121 ****
--- 139,157 ----
#define ALLOC_BLOCKHDRSZ MAXALIGN(sizeof(AllocBlockData))
#define ALLOC_CHUNKHDRSZ MAXALIGN(sizeof(AllocChunkData))
+ /* Portion of ALLOC_CHUNKHDRSZ examined outside aset.c. */
+ #define ALLOC_CHUNK_PUBLIC \
+ (offsetof(AllocChunkData, size) + sizeof(Size))
+
+ /* Portion of ALLOC_CHUNKHDRSZ excluding trailing padding. */
+ #ifdef MEMORY_CONTEXT_CHECKING
+ #define ALLOC_CHUNK_USED \
+ (offsetof(AllocChunkData, requested_size) + sizeof(Size))
+ #else
+ #define ALLOC_CHUNK_USED \
+ (offsetof(AllocChunkData, size) + sizeof(Size))
+ #endif
+
typedef struct AllocBlockData *AllocBlock; /* forward reference */
typedef struct AllocChunkData *AllocChunk;
***************
*** 314,320 **** AllocSetFreeIndex(Size size)
--- 350,358 ----
static void
wipe_mem(void *ptr, size_t size)
{
+ VALGRIND_MAKE_MEM_UNDEFINED(ptr, size);
memset(ptr, 0x7F, size);
+ VALGRIND_MAKE_MEM_NOACCESS(ptr, size);
}
#endif
***************
*** 324,330 **** set_sentinel(void *base, Size offset)
--- 362,370 ----
{
char *ptr = (char *) base + offset;
+ VALGRIND_MAKE_MEM_UNDEFINED(ptr, 1);
*ptr = 0x7E;
+ VALGRIND_MAKE_MEM_NOACCESS(ptr, 1);
}
static bool
***************
*** 333,339 **** sentinel_ok(const void *base, Size offset)
--- 373,381 ----
const char *ptr = (const char *) base + offset;
bool ret;
+ VALGRIND_MAKE_MEM_DEFINED(ptr, 1);
ret = *ptr == 0x7E;
+ VALGRIND_MAKE_MEM_NOACCESS(ptr, 1);
return ret;
}
***************
*** 346,365 **** sentinel_ok(const void *base, Size offset)
* very random, just a repeating sequence with a length that's prime. What
* we mainly want out of it is to have a good probability that two palloc's
* of the same number of bytes start out containing different data.
*/
static void
randomize_mem(char *ptr, size_t size)
{
static int save_ctr = 1;
int ctr;
ctr = save_ctr;
! while (size-- > 0)
{
*ptr++ = ctr;
if (++ctr > 251)
ctr = 1;
}
save_ctr = ctr;
}
#endif /* RANDOMIZE_ALLOCATED_MEMORY */
--- 388,416 ----
* very random, just a repeating sequence with a length that's prime. What
* we mainly want out of it is to have a good probability that two palloc's
* of the same number of bytes start out containing different data.
+ *
+ * The region may be NOACCESS, so make it UNDEFINED first to avoid errors as
+ * we fill it. Filling the region makes it DEFINED, so make it UNDEFINED
+ * again afterward. Whether to finally make it UNDEFINED or NOACCESS is
+ * fairly arbitrary. UNDEFINED is more convenient for AllocSetRealloc(), and
+ * other callers have no preference.
*/
static void
randomize_mem(char *ptr, size_t size)
{
static int save_ctr = 1;
+ size_t remaining = size;
int ctr;
ctr = save_ctr;
! VALGRIND_MAKE_MEM_UNDEFINED(ptr, size);
! while (remaining-- > 0)
{
*ptr++ = ctr;
if (++ctr > 251)
ctr = 1;
}
+ VALGRIND_MAKE_MEM_UNDEFINED(ptr - size, size);
save_ctr = ctr;
}
#endif /* RANDOMIZE_ALLOCATED_MEMORY */
***************
*** 455,460 **** AllocSetContextCreate(MemoryContext parent,
--- 506,515 ----
context->blocks = block;
/* Mark block as not to be released at reset time */
context->keeper = block;
+
+ /* Mark unallocated space NOACCESS; leave the block header
alone. */
+ VALGRIND_MAKE_MEM_NOACCESS(block->freeptr,
+ blksize -
ALLOC_BLOCKHDRSZ);
}
return (MemoryContext) context;
***************
*** 524,529 **** AllocSetReset(MemoryContext context)
--- 579,587 ----
#ifdef CLOBBER_FREED_MEMORY
wipe_mem(datastart, block->freeptr - datastart);
+ #else
+ /* wipe_mem() would have done this */
+ VALGRIND_MAKE_MEM_NOACCESS(datastart, block->freeptr -
datastart);
#endif
block->freeptr = datastart;
block->next = NULL;
***************
*** 623,628 **** AllocSetAlloc(MemoryContext context, Size size)
--- 681,687 ----
chunk->aset = set;
chunk->size = chunk_size;
#ifdef MEMORY_CONTEXT_CHECKING
+ /* Valgrind: Will be made NOACCESS below. */
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
if (size < chunk_size)
***************
*** 649,654 **** AllocSetAlloc(MemoryContext context, Size size)
--- 708,723 ----
}
AllocAllocInfo(set, chunk);
+
+ /*
+ * Chunk header public fields remain DEFINED. The requested
+ * allocation itself can be NOACCESS or UNDEFINED; our caller
will
+ * soon make it UNDEFINED. Make extra space at the end of the
chunk,
+ * if any, NOACCESS.
+ */
+ VALGRIND_MAKE_MEM_NOACCESS((char *) chunk + ALLOC_CHUNK_PUBLIC,
+ chunk_size + ALLOC_CHUNKHDRSZ
- ALLOC_CHUNK_PUBLIC);
+
return AllocChunkGetPointer(chunk);
}
***************
*** 669,675 **** AllocSetAlloc(MemoryContext context, Size size)
--- 738,747 ----
chunk->aset = (void *) set;
#ifdef MEMORY_CONTEXT_CHECKING
+ /* Valgrind: Free list requested_size should be DEFINED. */
chunk->requested_size = size;
+ VALGRIND_MAKE_MEM_NOACCESS(&chunk->requested_size,
+
sizeof(chunk->requested_size));
/* set mark to catch clobber of "unused" space */
if (size < chunk->size)
set_sentinel(AllocChunkGetPointer(chunk), size);
***************
*** 730,735 **** AllocSetAlloc(MemoryContext context, Size size)
--- 802,810 ----
chunk = (AllocChunk) (block->freeptr);
+ /* Prepare to initialize the chunk header. */
+ VALGRIND_MAKE_MEM_UNDEFINED(chunk,
ALLOC_CHUNK_USED);
+
block->freeptr += (availchunk +
ALLOC_CHUNKHDRSZ);
availspace -= (availchunk + ALLOC_CHUNKHDRSZ);
***************
*** 811,816 **** AllocSetAlloc(MemoryContext context, Size size)
--- 886,895 ----
if (set->keeper == NULL && blksize == set->initBlockSize)
set->keeper = block;
+ /* Mark unallocated space NOACCESS. */
+ VALGRIND_MAKE_MEM_NOACCESS(block->freeptr,
+ blksize -
ALLOC_BLOCKHDRSZ);
+
block->next = set->blocks;
set->blocks = block;
}
***************
*** 820,825 **** AllocSetAlloc(MemoryContext context, Size size)
--- 899,907 ----
*/
chunk = (AllocChunk) (block->freeptr);
+ /* Prepare to initialize the chunk header. */
+ VALGRIND_MAKE_MEM_UNDEFINED(chunk, ALLOC_CHUNK_USED);
+
block->freeptr += (chunk_size + ALLOC_CHUNKHDRSZ);
Assert(block->freeptr <= block->endptr);
***************
*** 827,832 **** AllocSetAlloc(MemoryContext context, Size size)
--- 909,916 ----
chunk->size = chunk_size;
#ifdef MEMORY_CONTEXT_CHECKING
chunk->requested_size = size;
+ VALGRIND_MAKE_MEM_NOACCESS(&chunk->requested_size,
+
sizeof(chunk->requested_size));
/* set mark to catch clobber of "unused" space */
if (size < chunk->size)
set_sentinel(AllocChunkGetPointer(chunk), size);
***************
*** 853,858 **** AllocSetFree(MemoryContext context, void *pointer)
--- 937,944 ----
AllocFreeInfo(set, chunk);
#ifdef MEMORY_CONTEXT_CHECKING
+ VALGRIND_MAKE_MEM_DEFINED(&chunk->requested_size,
+
sizeof(chunk->requested_size));
/* Test for someone scribbling on unused space in chunk */
if (chunk->requested_size < chunk->size)
if (!sentinel_ok(pointer, chunk->requested_size))
***************
*** 916,921 **** AllocSetFree(MemoryContext context, void *pointer)
--- 1002,1012 ----
* Returns new pointer to allocated memory of given size; this
memory
* is added to the set. Memory associated with given pointer is
copied
* into the new memory, and the old memory is freed.
+ *
+ * Without MEMORY_CONTEXT_CHECKING, we don't know the old request size. This
+ * makes our Valgrind client requests less-precise, hazarding false negatives.
+ * (In principle, we could use VALGRIND_GET_VBITS() to rediscover the old
+ * request size.)
*/
static void *
AllocSetRealloc(MemoryContext context, void *pointer, Size size)
***************
*** 925,930 **** AllocSetRealloc(MemoryContext context, void *pointer, Size
size)
--- 1016,1023 ----
Size oldsize = chunk->size;
#ifdef MEMORY_CONTEXT_CHECKING
+ VALGRIND_MAKE_MEM_DEFINED(&chunk->requested_size,
+
sizeof(chunk->requested_size));
/* Test for someone scribbling on unused space in chunk */
if (chunk->requested_size < oldsize)
if (!sentinel_ok(pointer, chunk->requested_size))
***************
*** 940,957 **** AllocSetRealloc(MemoryContext context, void *pointer, Size
size)
if (oldsize >= size)
{
#ifdef MEMORY_CONTEXT_CHECKING
#ifdef RANDOMIZE_ALLOCATED_MEMORY
/* We can only fill the extra space if we know the prior
request */
! if (size > chunk->requested_size)
! randomize_mem((char *) AllocChunkGetPointer(chunk) +
chunk->requested_size,
! size - chunk->requested_size);
#endif
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
if (size < oldsize)
set_sentinel(pointer, size);
#endif
return pointer;
}
--- 1033,1076 ----
if (oldsize >= size)
{
#ifdef MEMORY_CONTEXT_CHECKING
+ Size oldrequest = chunk->requested_size;
+
#ifdef RANDOMIZE_ALLOCATED_MEMORY
/* We can only fill the extra space if we know the prior
request */
! if (size > oldrequest)
! randomize_mem((char *) pointer + oldrequest,
! size - oldrequest);
#endif
chunk->requested_size = size;
+ VALGRIND_MAKE_MEM_NOACCESS(&chunk->requested_size,
+
sizeof(chunk->requested_size));
+
+ /*
+ * If this is an increase, mark any newly-available part
UNDEFINED.
+ * Otherwise, mark the obsolete part NOACCESS.
+ */
+ if (size > oldrequest)
+ VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer +
oldrequest,
+
size - oldrequest);
+ else
+ VALGRIND_MAKE_MEM_NOACCESS((char *) pointer + size,
+
oldsize - size);
+
/* set mark to catch clobber of "unused" space */
if (size < oldsize)
set_sentinel(pointer, size);
+ #else /*
!MEMORY_CONTEXT_CHECKING */
+
+ /*
+ * We don't have the information to determine whether we're
growing
+ * the old request or shrinking it, so we conservatively mark
the
+ * entire new allocation DEFINED.
+ */
+ VALGRIND_MAKE_MEM_NOACCESS(pointer, oldsize);
+ VALGRIND_MAKE_MEM_DEFINED(pointer, size);
#endif
+
return pointer;
}
***************
*** 997,1002 **** AllocSetRealloc(MemoryContext context, void *pointer, Size
size)
--- 1116,1122 ----
/* Update pointers since block has likely been moved */
chunk = (AllocChunk) (((char *) block) + ALLOC_BLOCKHDRSZ);
+ pointer = AllocChunkGetPointer(chunk);
if (prevblock == NULL)
set->blocks = block;
else
***************
*** 1006,1021 **** AllocSetRealloc(MemoryContext context, void *pointer, Size
size)
#ifdef MEMORY_CONTEXT_CHECKING
#ifdef RANDOMIZE_ALLOCATED_MEMORY
/* We can only fill the extra space if we know the prior
request */
! randomize_mem((char *) AllocChunkGetPointer(chunk) +
chunk->requested_size,
size - chunk->requested_size);
#endif
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
if (size < chunk->size)
set_sentinel(AllocChunkGetPointer(chunk), size);
#endif
return AllocChunkGetPointer(chunk);
}
else
--- 1126,1162 ----
#ifdef MEMORY_CONTEXT_CHECKING
#ifdef RANDOMIZE_ALLOCATED_MEMORY
/* We can only fill the extra space if we know the prior
request */
! randomize_mem((char *) pointer + chunk->requested_size,
size - chunk->requested_size);
#endif
+ /*
+ * realloc() (or randomize_mem()) will have left the
newly-allocated
+ * part UNDEFINED, but we may need to adjust trailing bytes
from the
+ * old allocation.
+ */
+ VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer +
chunk->requested_size,
+ oldsize
- chunk->requested_size);
+
chunk->requested_size = size;
+ VALGRIND_MAKE_MEM_NOACCESS(&chunk->requested_size,
+
sizeof(chunk->requested_size));
+
/* set mark to catch clobber of "unused" space */
if (size < chunk->size)
set_sentinel(AllocChunkGetPointer(chunk), size);
+ #else /*
!MEMORY_CONTEXT_CHECKING */
+
+ /*
+ * We don't know how much of the old chunk size was the actual
+ * allocation; it could have been as small as one byte. We
have to be
+ * conservative and just mark the entire old portion DEFINED.
+ */
+ VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
#endif
+ /* Make any trailing alignment padding NOACCESS. */
+ VALGRIND_MAKE_MEM_NOACCESS((char *) pointer + size, chksize -
size);
return AllocChunkGetPointer(chunk);
}
else
***************
*** 1036,1041 **** AllocSetRealloc(MemoryContext context, void *pointer, Size
size)
--- 1177,1196 ----
/* allocate new chunk */
newPointer = AllocSetAlloc((MemoryContext) set, size);
+ /*
+ * AllocSetAlloc() just made the region NOACCESS. Change it to
+ * UNDEFINED for the moment; memcpy() will then transfer
definedness
+ * from the old allocation to the new. If we know the old
allocation,
+ * copy just that much. Otherwise, make the entire old chunk
defined
+ * to avoid errors as we copy the currently-NOACCESS trailing
bytes.
+ */
+ VALGRIND_MAKE_MEM_UNDEFINED(newPointer, size);
+ #ifdef MEMORY_CONTEXT_CHECKING
+ oldsize = chunk->requested_size;
+ #else
+ VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
+ #endif
+
/* transfer existing data (certain to fit) */
memcpy(newPointer, pointer, oldsize);
***************
*** 1164,1170 **** AllocSetCheck(MemoryContext context)
--- 1319,1330 ----
dsize;
chsize = chunk->size; /* aligned chunk size */
+ VALGRIND_MAKE_MEM_DEFINED(&chunk->requested_size,
+
sizeof(chunk->requested_size));
dsize = chunk->requested_size; /* real data */
+ if (dsize > 0) /* not on a free list */
+
VALGRIND_MAKE_MEM_NOACCESS(&chunk->requested_size,
+
sizeof(chunk->requested_size));
/*
* Check chunk size
*** a/src/backend/utils/mmgr/mcxt.c
--- b/src/backend/utils/mmgr/mcxt.c
***************
*** 24,29 ****
--- 24,30 ----
#include "postgres.h"
+ #include "utils/memdebug.h"
#include "utils/memutils.h"
***************
*** 135,140 **** MemoryContextReset(MemoryContext context)
--- 136,143 ----
{
(*context->methods->reset) (context);
context->isReset = true;
+ VALGRIND_DESTROY_MEMPOOL(context);
+ VALGRIND_CREATE_MEMPOOL(context, 0, false);
}
}
***************
*** 184,189 **** MemoryContextDelete(MemoryContext context)
--- 187,193 ----
MemoryContextSetParent(context, NULL);
(*context->methods->delete_context) (context);
+ VALGRIND_DESTROY_MEMPOOL(context);
pfree(context);
}
***************
*** 555,560 **** MemoryContextCreate(NodeTag tag, Size size,
--- 559,566 ----
parent->firstchild = node;
}
+ VALGRIND_CREATE_MEMPOOL(node, 0, false);
+
/* Return to type-specific creation routine to finish up */
return node;
}
***************
*** 580,585 **** MemoryContextAlloc(MemoryContext context, Size size)
--- 586,592 ----
context->isReset = false;
ret = (*context->methods->alloc) (context, size);
+ VALGRIND_MEMPOOL_ALLOC(context, ret, size);
return ret;
}
***************
*** 605,610 **** MemoryContextAllocZero(MemoryContext context, Size size)
--- 612,618 ----
context->isReset = false;
ret = (*context->methods->alloc) (context, size);
+ VALGRIND_MEMPOOL_ALLOC(context, ret, size);
MemSetAligned(ret, 0, size);
***************
*** 632,637 **** MemoryContextAllocZeroAligned(MemoryContext context, Size size)
--- 640,646 ----
context->isReset = false;
ret = (*context->methods->alloc) (context, size);
+ VALGRIND_MEMPOOL_ALLOC(context, ret, size);
MemSetLoop(ret, 0, size);
***************
*** 653,658 **** palloc(Size size)
--- 662,668 ----
CurrentMemoryContext->isReset = false;
ret = (*CurrentMemoryContext->methods->alloc) (CurrentMemoryContext,
size);
+ VALGRIND_MEMPOOL_ALLOC(CurrentMemoryContext, ret, size);
return ret;
}
***************
*** 672,677 **** palloc0(Size size)
--- 682,688 ----
CurrentMemoryContext->isReset = false;
ret = (*CurrentMemoryContext->methods->alloc) (CurrentMemoryContext,
size);
+ VALGRIND_MEMPOOL_ALLOC(CurrentMemoryContext, ret, size);
MemSetAligned(ret, 0, size);
***************
*** 704,709 **** pfree(void *pointer)
--- 715,721 ----
AssertArg(MemoryContextIsValid(context));
(*context->methods->free_p) (context, pointer);
+ VALGRIND_MEMPOOL_FREE(context, pointer);
}
/*
***************
*** 740,745 **** repalloc(void *pointer, Size size)
--- 752,758 ----
Assert(!context->isReset);
ret = (*context->methods->realloc) (context, pointer, size);
+ VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size);
return ret;
}
*** a/src/include/pg_config_manual.h
--- b/src/include/pg_config_manual.h
***************
*** 207,212 ****
--- 207,227 ----
*/
/*
+ * Include Valgrind "client requests", mostly in the memory allocator, so
+ * Valgrind understands PostgreSQL memory contexts. This permits detecting
+ * memory errors that Valgrind would not detect on a vanilla build. See also
+ * src/tools/valgrind.supp. "make installcheck" runs 20-30x longer under
+ * Valgrind. Note that USE_VALGRIND slowed older versions of Valgrind by an
+ * additional order of magnitude; Valgrind 3.8.1 does not have this problem.
+ * The client requests fall in hot code paths, so USE_VALGRIND also slows
+ * native execution by a few percentage points.
+ *
+ * You should normally use MEMORY_CONTEXT_CHECKING with USE_VALGRIND;
+ * instrumentation of repalloc() is inferior without it.
+ */
+ /* #define USE_VALGRIND */
+
+ /*
* Define this to cause pfree()'d memory to be cleared immediately, to
* facilitate catching bugs that refer to already-freed values.
* Right now, this gets defined automatically if --enable-cassert.
***************
*** 218,226 ****
/*
* Define this to check memory allocation errors (scribbling on more
* bytes than were allocated). Right now, this gets defined
! * automatically if --enable-cassert.
*/
! #ifdef USE_ASSERT_CHECKING
#define MEMORY_CONTEXT_CHECKING
#endif
--- 233,241 ----
/*
* Define this to check memory allocation errors (scribbling on more
* bytes than were allocated). Right now, this gets defined
! * automatically if --enable-cassert or USE_VALGRIND.
*/
! #if defined(USE_ASSERT_CHECKING) || defined(USE_VALGRIND)
#define MEMORY_CONTEXT_CHECKING
#endif
*** /dev/null
--- b/src/include/utils/memdebug.h
***************
*** 0 ****
--- 1,34 ----
+ /*-------------------------------------------------------------------------
+ *
+ * memdebug.h
+ * Memory debugging support.
+ *
+ * Currently, this file either wraps <valgrind/memcheck.h> or substitutes
+ * empty definitions for Valgrind client request macros we use.
+ *
+ *
+ * Portions Copyright (c) 1996-2013, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/utils/memdebug.h
+ *
+ *-------------------------------------------------------------------------
+ */
+ #ifndef MEMDEBUG_H
+ #define MEMDEBUG_H
+
+ #ifdef USE_VALGRIND
+ #include <valgrind/memcheck.h>
+ #else
+ #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_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)
+ #define VALGRIND_MEMPOOL_ALLOC(context, addr, size) do {}
while (0)
+ #define VALGRIND_MEMPOOL_FREE(context, addr) do {}
while (0)
+ #define VALGRIND_MEMPOOL_CHANGE(context, optr, nptr, size) do {} while (0)
+ #endif
+
+ #endif /* MEMDEBUG_H */
*** /dev/null
--- b/src/tools/valgrind.supp
***************
*** 0 ****
--- 1,94 ----
+ # This is a suppression file for use with Valgrind tools. File format
+ # documentation:
+ # http://valgrind.org/docs/manual/mc-manual.html#mc-manual.suppfiles
+
+ # The libc symbol that implements a particular standard interface is
+ # implementation-dependent. For example, strncpy() shows up as "__GI_strncpy"
+ # on some platforms. Use wildcards to avoid mentioning such specific names.
+
+
+ # We have occasion to write raw binary structures to disk or to the network.
+ # These may contain uninitialized padding bytes. Since recipients also ignore
+ # those bytes as padding, this is harmless.
+
+ {
+ padding_pgstat_send
+ Memcheck:Param
+ socketcall.send(msg)
+
+ fun:*send*
+ fun:pgstat_send
+ }
+
+ {
+ padding_pgstat_sendto
+ Memcheck:Param
+ socketcall.sendto(msg)
+
+ fun:*send*
+ fun:pgstat_send
+ }
+
+ {
+ padding_pgstat_write
+ Memcheck:Param
+ write(buf)
+
+ ...
+ fun:pgstat_write_statsfiles
+ }
+
+ {
+ padding_XLogRecData_CRC
+ Memcheck:Value8
+
+ fun:XLogInsert
+ }
+
+ {
+ padding_XLogRecData_write
+ Memcheck:Param
+ write(buf)
+
+ ...
+ fun:XLogWrite
+ }
+
+ {
+ padding_relcache
+ Memcheck:Param
+ write(buf)
+
+ ...
+ fun:write_relcache_init_file
+ }
+
+
+ # resolve_polymorphic_tupdesc(), a subroutine of internal_get_result_type(),
+ # can instigate a memcpy() call where the two pointer arguments are exactly
+ # equal. The behavior thereof is formally undefined, but implementations
+ # where it's anything other than a no-op are thought unlikely.
+ {
+ noopmemcpy_internal_get_result_type
+ Memcheck:Overlap
+
+ fun:*strncpy*
+ fun:namestrcpy
+ fun:TupleDescInitEntry
+ ...
+ fun:internal_get_result_type
+ }
+
+
+ # gcc on ppc64 can generate a four-byte read to fetch the final "char" fields
+ # of a FormData_pg_cast. This is valid compiler behavior, because a proper
+ # FormData_pg_cast has trailing padding. Tuples we treat as structures omit
+ # that padding, so Valgrind reports an invalid read. Practical trouble would
+ # entail the missing pad bytes falling in a different memory page. So long as
+ # the structure is aligned, that will not happen.
+ {
+ overread_tuplestruct_pg_cast
+ Memcheck:Addr4
+
+ fun:IsBinaryCoercible
+ }
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
