Improve valgrind logic in aset.c, and fix multiple issues in generation.c.

Revise aset.c so that all the "private" fields of chunk headers are marked NOACCESS when outside the module, improving on the previous coding which protected only requested_size. Fix a couple of corner case bugs, such as failing to re-protect the header during a failure exit from AllocSetRealloc, and wrong padding-size calculation for an oversize allocation request. Apply the same design to generation.c, and also fix several bugs therein that I found by dint of hacking the code to use generation.c as the standard allocator and then running the core regression tests with it. Notably, we have to track the actual size of each block, else the wipe_mem call in GenerationReset clears the wrong amount of memory for an oversize-chunk block; and GenerationCheck needs a way of identifying freed chunks that isn't fooled by palloc(0). I chose to fix the latter by resetting the context pointer to NULL in a freed chunk, roughly like what happens in a freed aset.c chunk. Discussion: https://postgr.es/m/E1eHa4J-0006hI-Q8@gemulon.postgresql.org
2025-11-19 13:42:17 +03:00 · 2017-11-24 19:28:19 -05:00
parent f65d21b258
commit 0f2458ff5f
2 changed files with 192 additions and 102 deletions
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -190,6 +190,14 @@ typedef struct AllocChunkData
 	/* there must not be any padding to reach a MAXALIGN boundary here! */
 }			AllocChunkData;

+/*
+ * Only the "aset" 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(AllocChunkData, aset)
+
 /*
 * AllocPointerIsValid
 *		True iff pointer is valid allocation pointer.
@@ -572,6 +580,10 @@ AllocSetDelete(MemoryContext context)
 * No request may exceed:
 *		MAXALIGN_DOWN(SIZE_MAX) - ALLOC_BLOCKHDRSZ - ALLOC_CHUNKHDRSZ
 * All callers use a much-lower limit.
+ *
+ * Note: when using valgrind, it doesn't matter how the returned allocation
+ * is marked, as mcxt.c will set it to UNDEFINED.  In some paths we will
+ * return space that is marked NOACCESS - AllocSetRealloc has to beware!
 */
 static void *
 AllocSetAlloc(MemoryContext context, Size size)
@@ -603,7 +615,6 @@ AllocSetAlloc(MemoryContext context, Size size)
 		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)
@@ -635,14 +646,12 @@ AllocSetAlloc(MemoryContext context, Size size)

 		AllocAllocInfo(set, chunk);

-		/*
-		 * Chunk's metadata 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_CHUNKHDRSZ,
-								   chunk_size - ALLOC_CHUNKHDRSZ);
+		/* Ensure any padding bytes are marked NOACCESS. */
+		VALGRIND_MAKE_MEM_NOACCESS((char *) AllocChunkGetPointer(chunk) + size,
+								   chunk_size - size);
+
+		/* Disallow external access to private part of chunk header. */
+		VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);

 		return AllocChunkGetPointer(chunk);
 	}
@@ -664,10 +673,7 @@ AllocSetAlloc(MemoryContext context, Size size)
 		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);
@@ -678,6 +684,14 @@ AllocSetAlloc(MemoryContext context, Size size)
 #endif

 		AllocAllocInfo(set, chunk);
+
+		/* Ensure any padding bytes are marked NOACCESS. */
+		VALGRIND_MAKE_MEM_NOACCESS((char *) AllocChunkGetPointer(chunk) + size,
+								   chunk->size - size);
+
+		/* Disallow external access to private part of chunk header. */
+		VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+
 		return AllocChunkGetPointer(chunk);
 	}

@@ -831,8 +845,6 @@ AllocSetAlloc(MemoryContext context, Size size)
 	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);
@@ -843,6 +855,14 @@ AllocSetAlloc(MemoryContext context, Size size)
 #endif

 	AllocAllocInfo(set, chunk);
+
+	/* Ensure any padding bytes are marked NOACCESS. */
+	VALGRIND_MAKE_MEM_NOACCESS((char *) AllocChunkGetPointer(chunk) + size,
+							   chunk_size - size);
+
+	/* Disallow external access to private part of chunk header. */
+	VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+
 	return AllocChunkGetPointer(chunk);
 }

@@ -856,11 +876,12 @@ AllocSetFree(MemoryContext context, void *pointer)
 	AllocSet	set = (AllocSet) context;
 	AllocChunk	chunk = AllocPointerGetChunk(pointer);

+	/* Allow access to private part of chunk header. */
+	VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
+
 	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))
@@ -935,11 +956,14 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
 {
 	AllocSet	set = (AllocSet) context;
 	AllocChunk	chunk = AllocPointerGetChunk(pointer);
-	Size		oldsize = chunk->size;
+	Size		oldsize;
+
+	/* Allow access to private part of chunk header. */
+	VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
+
+	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))
@@ -965,8 +989,6 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
 #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.
@@ -993,6 +1015,9 @@ AllocSetRealloc(MemoryContext context, 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);
+
 		return pointer;
 	}

@@ -1023,7 +1048,11 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
 		blksize = chksize + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ;
 		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);
 			return NULL;
+		}
 		block->freeptr = block->endptr = ((char *) block) + blksize;

 		/* Update pointers since block has likely been moved */
@@ -1053,8 +1082,6 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size 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)
@@ -1069,9 +1096,12 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
 		VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
 #endif

-		/* Make any trailing alignment padding NOACCESS. */
+		/* 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);
+
 		return pointer;
 	}
 	else
@@ -1094,14 +1124,19 @@ AllocSetRealloc(MemoryContext context, 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);
 			return NULL;
+		}

 		/*
-		 * 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.
+		 * AllocSetAlloc() may have returned a region that is still 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
@@ -1129,8 +1164,12 @@ static Size
 AllocSetGetChunkSpace(MemoryContext context, void *pointer)
 {
 	AllocChunk	chunk = AllocPointerGetChunk(pointer);
+	Size		result;

-	return chunk->size + ALLOC_CHUNKHDRSZ;
+	VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
+	result = chunk->size + ALLOC_CHUNKHDRSZ;
+	VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+	return result;
 }

 /*
@@ -1267,13 +1306,11 @@ AllocSetCheck(MemoryContext context)
 			Size		chsize,
 						dsize;

+			/* Allow access to private part of chunk header. */
+			VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
+
 			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
@@ -1294,20 +1331,28 @@ AllocSetCheck(MemoryContext context)
 			/*
 			 * If chunk is allocated, check for correct aset pointer. (If it's
 			 * free, the aset is the freelist pointer, which we can't check as
-			 * easily...)
+			 * easily...)  Note this is an incomplete test, since palloc(0)
+			 * produces an allocated chunk with requested_size == 0.
 			 */
 			if (dsize > 0 && chunk->aset != (void *) set)
 				elog(WARNING, "problem in alloc set %s: bogus aset link in block %p, chunk %p",
 					 name, block, chunk);

 			/*
-			 * Check for overwrite of "unallocated" space in chunk
+			 * Check for overwrite of padding space in an allocated chunk.
 			 */
-			if (dsize > 0 && dsize < chsize &&
+			if (chunk->aset == (void *) set && 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);

+			/*
+			 * If chunk is allocated, disallow external access to private part
+			 * of chunk header.
+			 */
+			if (chunk->aset == (void *) set)
+				VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
+
 			blk_data += chsize;
 			nchunks++;