So I pushed that, but I don't feel that we're out of the woods yet.
As I mentioned at [1], while testing this stuff I hit a case where
aset.c will try to wipe_mem practically the entire address space after
being asked to pfree() an invalid pointer. The specific reproducer
isn't too interesting because it depends on the pre-80ef92675 state of
the code, but what I take away from this is that aset.c is still far
too fragile as it stands.
One problem is that aset.c generally isn't too careful about whether
a putative chunk is actually one of its chunks. That was okay before
c6e0fe1f2 because we would never get to AllocSetFree etc unless the
word before the chunk data pointed at a moderately-sane AllocSet.
Now, we can arrive at that code on the strength of three random bits,
so it's got to be more careful. In the attached patch, I make
AllocSetIsValid do an IsA() test, and make sure to apply it to the
aset we think we have found from the chunk header. This is not in
any way a new check: what it is doing is replacing the IsA check done
by the "AssertArg(MemoryContextIsValid(context))" that was performed
by GetMemoryChunkContext in the old code, and that we've completely
lost in the code as it stands.
The other problem, which is what is leading to wipe_mem-the-universe,
is that aset.c figures the size of a non-external chunk essentially
as "1 << MemoryChunkGetValue(chunk)", where the "value" field is 30
bits wide and has undergone exactly zero validation before
AllocSetFree uses the size in memset. That's far, far too trusting.
In the attached I put in some asserts to verify that the value field
is in the valid range for a freelist index, which should usually
trigger if we have a garbage value, or at the very least constrain
the damage.
What I am mainly wondering about at this point is whether Asserts
are good enough or we should use actual test-and-elog checks for
these things. The precedent of the old GetMemoryChunkContext
implementation suggests that assertions are good enough for the
AllocSetIsValid tests. On the other hand, there are existing
cross-checks like
if (block->freeptr != block->endptr)
elog(ERROR, "could not find block containing chunk %p", chunk);
so at least in some code paths we've thought it is worth expending
such tests in production builds. Any opinions?
I imagine generation.c and slab.c need similar bulletproofing
measures, but I didn't look at them yet.
regards, tom lane
[1] https://www.postgresql.org/message-id/3436789.1665187055%40sss.pgh.pa.us
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec423375ae..76a9f02bd8 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -132,6 +132,10 @@ typedef struct AllocFreeListLink
#define GetFreeListLink(chkptr) \
(AllocFreeListLink *) ((char *) (chkptr) + ALLOC_CHUNKHDRSZ)
+/* Validate a freelist index retrieved from a chunk header */
+#define FreeListIdxIsValid(fidx) \
+ ((fidx) >= 0 && (fidx) < ALLOCSET_NUM_FREELISTS)
+
/* Determine the size of the chunk based on the freelist index */
#define GetChunkSizeFromFreeListIdx(fidx) \
((((Size) 1) << ALLOC_MINBITS) << (fidx))
@@ -202,7 +206,15 @@ typedef struct AllocBlockData
* AllocSetIsValid
* True iff set is valid allocation set.
*/
-#define AllocSetIsValid(set) PointerIsValid(set)
+#define AllocSetIsValid(set) \
+ (PointerIsValid(set) && IsA(set, AllocSetContext))
+
+/*
+ * AllocBlockIsValid
+ * True iff block is valid block of allocation set.
+ */
+#define AllocBlockIsValid(block) \
+ (PointerIsValid(block) && AllocSetIsValid((block)->aset))
/*
* We always store external chunks on a dedicated block. This makes fetching
@@ -530,8 +542,7 @@ AllocSetReset(MemoryContext context)
{
AllocSet set = (AllocSet) context;
AllocBlock block;
- Size keepersize PG_USED_FOR_ASSERTS_ONLY
- = set->keeper->endptr - ((char *) set);
+ Size keepersize PG_USED_FOR_ASSERTS_ONLY;
AssertArg(AllocSetIsValid(set));
@@ -540,6 +551,9 @@ AllocSetReset(MemoryContext context)
AllocSetCheck(context);
#endif
+ /* Remember keeper block size for Assert below */
+ keepersize = set->keeper->endptr - ((char *) set);
+
/* Clear chunk freelists */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
@@ -598,8 +612,7 @@ AllocSetDelete(MemoryContext context)
{
AllocSet set = (AllocSet) context;
AllocBlock block = set->blocks;
- Size keepersize PG_USED_FOR_ASSERTS_ONLY
- = set->keeper->endptr - ((char *) set);
+ Size keepersize PG_USED_FOR_ASSERTS_ONLY;
AssertArg(AllocSetIsValid(set));
@@ -608,6 +621,9 @@ AllocSetDelete(MemoryContext context)
AllocSetCheck(context);
#endif
+ /* Remember keeper block size for Assert below */
+ keepersize = set->keeper->endptr - ((char *) set);
+
/*
* If the context is a candidate for a freelist, put it into that freelist
* instead of destroying it.
@@ -994,9 +1010,10 @@ AllocSetFree(void *pointer)
if (MemoryChunkIsExternal(chunk))
{
-
+ /* Release single-chunk block. */
AllocBlock block = ExternalChunkGetBlock(chunk);
+ AssertArg(AllocBlockIsValid(block));
set = block->aset;
#ifdef MEMORY_CONTEXT_CHECKING
@@ -1011,7 +1028,6 @@ AllocSetFree(void *pointer)
}
#endif
-
/*
* Try to verify that we have a sane block pointer, the freeptr should
* match the endptr.
@@ -1036,12 +1052,17 @@ AllocSetFree(void *pointer)
}
else
{
- int fidx = MemoryChunkGetValue(chunk);
AllocBlock block = MemoryChunkGetBlock(chunk);
- AllocFreeListLink *link = GetFreeListLink(chunk);
+ int fidx;
+ AllocFreeListLink *link;
+ AssertArg(AllocBlockIsValid(block));
set = block->aset;
+ fidx = MemoryChunkGetValue(chunk);
+ Assert(FreeListIdxIsValid(fidx));
+ link = GetFreeListLink(chunk);
+
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
if (chunk->requested_size < GetChunkSizeFromFreeListIdx(fidx))
@@ -1089,6 +1110,7 @@ AllocSetRealloc(void *pointer, Size size)
AllocSet set;
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
Size oldsize;
+ int fidx;
/* Allow access to private part of chunk header. */
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
@@ -1105,9 +1127,11 @@ AllocSetRealloc(void *pointer, Size size)
Size oldblksize;
block = ExternalChunkGetBlock(chunk);
- oldsize = block->endptr - (char *) pointer;
+ AssertArg(AllocBlockIsValid(block));
set = block->aset;
+ oldsize = block->endptr - (char *) pointer;
+
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
Assert(chunk->requested_size < oldsize);
@@ -1201,9 +1225,13 @@ AllocSetRealloc(void *pointer, Size size)
}
block = MemoryChunkGetBlock(chunk);
- oldsize = GetChunkSizeFromFreeListIdx(MemoryChunkGetValue(chunk));
+ AssertArg(AllocBlockIsValid(block));
set = block->aset;
+ fidx = MemoryChunkGetValue(chunk);
+ Assert(FreeListIdxIsValid(fidx));
+ oldsize = GetChunkSizeFromFreeListIdx(fidx);
+
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
if (chunk->requested_size < oldsize)
@@ -1328,6 +1356,7 @@ AllocSetGetChunkContext(void *pointer)
else
block = (AllocBlock) MemoryChunkGetBlock(chunk);
+ AssertArg(AllocBlockIsValid(block));
set = block->aset;
return &set->header;
@@ -1342,16 +1371,19 @@ Size
AllocSetGetChunkSpace(void *pointer)
{
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
+ int fidx;
if (MemoryChunkIsExternal(chunk))
{
AllocBlock block = ExternalChunkGetBlock(chunk);
+ AssertArg(AllocBlockIsValid(block));
return block->endptr - (char *) chunk;
}
- return GetChunkSizeFromFreeListIdx(MemoryChunkGetValue(chunk)) +
- ALLOC_CHUNKHDRSZ;
+ fidx = MemoryChunkGetValue(chunk);
+ Assert(FreeListIdxIsValid(fidx));
+ return GetChunkSizeFromFreeListIdx(fidx) + ALLOC_CHUNKHDRSZ;
}
/*
@@ -1361,6 +1393,8 @@ AllocSetGetChunkSpace(void *pointer)
bool
AllocSetIsEmpty(MemoryContext context)
{
+ AssertArg(AllocSetIsValid(context));
+
/*
* For now, we say "empty" only if the context is new or just reset. We
* could examine the freelists to determine if all space has been freed,
@@ -1394,6 +1428,8 @@ AllocSetStats(MemoryContext context,
AllocBlock block;
int fidx;
+ AssertArg(AllocSetIsValid(set));
+
/* Include context header in totalspace */
totalspace = MAXALIGN(sizeof(AllocSetContext));
@@ -1405,14 +1441,14 @@ AllocSetStats(MemoryContext context,
}
for (fidx = 0; fidx < ALLOCSET_NUM_FREELISTS; fidx++)
{
+ Size chksz = GetChunkSizeFromFreeListIdx(fidx);
MemoryChunk *chunk = set->freelist[fidx];
while (chunk != NULL)
{
- Size chksz = GetChunkSizeFromFreeListIdx(MemoryChunkGetValue(chunk));
AllocFreeListLink *link = GetFreeListLink(chunk);
- Assert(GetChunkSizeFromFreeListIdx(fidx) == chksz);
+ Assert(MemoryChunkGetValue(chunk) == fidx);
freechunks++;
freespace += chksz + ALLOC_CHUNKHDRSZ;
@@ -1522,7 +1558,13 @@ AllocSetCheck(MemoryContext context)
}
else
{
- chsize = GetChunkSizeFromFreeListIdx(MemoryChunkGetValue(chunk)); /* aligned chunk size */
+ int fidx = MemoryChunkGetValue(chunk);
+
+ if (!FreeListIdxIsValid(fidx))
+ elog(WARNING, "problem in alloc set %s: bad chunk size for chunk %p in block %p",
+ name, chunk, block);
+
+ chsize = GetChunkSizeFromFreeListIdx(fidx); /* aligned chunk size */
/*
* Check the stored block offset correctly references this
