From a8bc48b660604a846c6a84ad77cd407236401c4e Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Tue, 26 May 2026 16:52:19 -0400 Subject: [PATCH v2 1/3] WIP: mmgr: Use larger and guaranteed to exist sentinel --- src/include/pg_config_manual.h | 11 ++++ src/include/utils/memdebug.h | 21 +++++--- src/backend/utils/mmgr/aset.c | 82 +++++++++++++++-------------- src/backend/utils/mmgr/bump.c | 20 +++---- src/backend/utils/mmgr/generation.c | 20 +++---- src/backend/utils/mmgr/mcxt.c | 6 +-- src/backend/utils/mmgr/slab.c | 16 +++--- 7 files changed, 88 insertions(+), 88 deletions(-) diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h index 521b49b8888..25b77470cef 100644 --- a/src/include/pg_config_manual.h +++ b/src/include/pg_config_manual.h @@ -278,6 +278,17 @@ #define MEMORY_CONTEXT_CHECKING #endif +/* + * Number of bytes to extend each allocation by, to check for buffer + * overflows. It's probably good for this to be >= sizeof(size_t), to be able + * to detect overflows in arrays. + */ +#if defined(USE_ASSERT_CHECKING) || defined(USE_VALGRIND) +#define MEMORY_CONTEXT_SENTINEL_SIZE 16 +#else +#define MEMORY_CONTEXT_SENTINEL_SIZE 0 +#endif + /* * Define this to cause palloc()'d memory to be filled with random data, to * facilitate catching code that depends on the contents of uninitialized diff --git a/src/include/utils/memdebug.h b/src/include/utils/memdebug.h index ceb6864d435..4e73e6fbb00 100644 --- a/src/include/utils/memdebug.h +++ b/src/include/utils/memdebug.h @@ -53,20 +53,27 @@ set_sentinel(void *base, Size offset) { char *ptr = (char *) base + offset; - VALGRIND_MAKE_MEM_UNDEFINED(ptr, 1); - *ptr = 0x7E; - VALGRIND_MAKE_MEM_NOACCESS(ptr, 1); + VALGRIND_MAKE_MEM_UNDEFINED(ptr, MEMORY_CONTEXT_SENTINEL_SIZE); + memset(ptr, 0x7E, MEMORY_CONTEXT_SENTINEL_SIZE); + VALGRIND_MAKE_MEM_NOACCESS(ptr, MEMORY_CONTEXT_SENTINEL_SIZE); } static inline bool sentinel_ok(const void *base, Size offset) { const char *ptr = (const char *) base + offset; - bool ret; + bool ret = true; - VALGRIND_MAKE_MEM_DEFINED(ptr, 1); - ret = *ptr == 0x7E; - VALGRIND_MAKE_MEM_NOACCESS(ptr, 1); + VALGRIND_MAKE_MEM_DEFINED(ptr, MEMORY_CONTEXT_SENTINEL_SIZE); + for (Size i = 0; i < MEMORY_CONTEXT_SENTINEL_SIZE; i++) + { + if (*ptr++ != 0x7e) + { + ret = false; + break; + } + } + VALGRIND_MAKE_MEM_NOACCESS(ptr, MEMORY_CONTEXT_SENTINEL_SIZE); return ret; } diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c index 6a9ea367107..75eb294e9da 100644 --- a/src/backend/utils/mmgr/aset.c +++ b/src/backend/utils/mmgr/aset.c @@ -743,12 +743,8 @@ AllocSetAllocLarge(MemoryContext context, Size size, int flags) /* validate 'size' is within the limits for the given 'flags' */ MemoryContextCheckSize(context, size, flags); -#ifdef MEMORY_CONTEXT_CHECKING - /* ensure there's always space for the sentinel byte */ - chunk_size = MAXALIGN(size + 1); -#else - chunk_size = MAXALIGN(size); -#endif + /* ensure there's space for the sentinel, if needed */ + chunk_size = MAXALIGN(size + MEMORY_CONTEXT_SENTINEL_SIZE); blksize = chunk_size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ; block = (AllocBlock) malloc(blksize); @@ -771,7 +767,7 @@ AllocSetAllocLarge(MemoryContext context, Size size, int flags) #ifdef MEMORY_CONTEXT_CHECKING chunk->requested_size = size; /* set mark to catch clobber of "unused" space */ - Assert(size < chunk_size); + Assert(size + MEMORY_CONTEXT_SENTINEL_SIZE <= chunk_size); set_sentinel(MemoryChunkGetPointer(chunk), size); #endif #ifdef RANDOMIZE_ALLOCATED_MEMORY @@ -820,6 +816,13 @@ AllocSetAllocChunkFromBlock(MemoryContext context, AllocBlock block, chunk = (MemoryChunk *) (block->freeptr); + /* + * Ensure there's space for the sentinel, if needed. Note this is + * intentionally done after determining fidx, the space for the sentinel + * is in addition to what the size class guarantees. + */ + chunk_size += MEMORY_CONTEXT_SENTINEL_SIZE; + /* Prepare to initialize the chunk header. */ VALGRIND_MAKE_MEM_UNDEFINED(chunk, ALLOC_CHUNKHDRSZ); @@ -832,8 +835,8 @@ AllocSetAllocChunkFromBlock(MemoryContext context, AllocBlock block, #ifdef MEMORY_CONTEXT_CHECKING chunk->requested_size = size; /* set mark to catch clobber of "unused" space */ - if (size < chunk_size) - set_sentinel(MemoryChunkGetPointer(chunk), size); + Assert(size + MEMORY_CONTEXT_SENTINEL_SIZE <= chunk_size); + set_sentinel(MemoryChunkGetPointer(chunk), size); #endif #ifdef RANDOMIZE_ALLOCATED_MEMORY /* fill the allocated space with junk */ @@ -884,11 +887,11 @@ AllocSetAllocFromNewBlock(MemoryContext context, Size size, int flags, * left in the block, this loop cannot iterate more than * ALLOCSET_NUM_FREELISTS-1 times. */ - while (availspace >= ((1 << ALLOC_MINBITS) + ALLOC_CHUNKHDRSZ)) + while (availspace >= ((1 << ALLOC_MINBITS) + ALLOC_CHUNKHDRSZ + MEMORY_CONTEXT_SENTINEL_SIZE)) { AllocFreeListLink *link; MemoryChunk *chunk; - Size availchunk = availspace - ALLOC_CHUNKHDRSZ; + Size availchunk = availspace - ALLOC_CHUNKHDRSZ - MEMORY_CONTEXT_SENTINEL_SIZE; int a_fidx = AllocSetFreeIndex(availchunk); /* @@ -907,8 +910,8 @@ AllocSetAllocFromNewBlock(MemoryContext context, Size size, int flags, /* Prepare to initialize the chunk header. */ VALGRIND_MAKE_MEM_UNDEFINED(chunk, ALLOC_CHUNKHDRSZ); - block->freeptr += (availchunk + ALLOC_CHUNKHDRSZ); - availspace -= (availchunk + ALLOC_CHUNKHDRSZ); + block->freeptr += (availchunk + ALLOC_CHUNKHDRSZ + MEMORY_CONTEXT_SENTINEL_SIZE); + availspace -= (availchunk + ALLOC_CHUNKHDRSZ + MEMORY_CONTEXT_SENTINEL_SIZE); /* store the freelist index in the value field */ MemoryChunkSetHdrMask(chunk, block, a_fidx, MCTX_ASET_ID); @@ -942,7 +945,8 @@ AllocSetAllocFromNewBlock(MemoryContext context, Size size, int flags, * If initBlockSize is less than ALLOC_CHUNK_LIMIT, we could need more * space... but try to keep it a power of 2. */ - required_size = chunk_size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ; + required_size = chunk_size + ALLOC_BLOCKHDRSZ + + ALLOC_CHUNKHDRSZ + MEMORY_CONTEXT_SENTINEL_SIZE; while (blksize < required_size) blksize <<= 1; @@ -1036,11 +1040,10 @@ AllocSetAlloc(MemoryContext context, Size size, int flags) * If one is found, remove it from the free list, make it again a member * of the alloc set and return its data address. * - * Note that we don't attempt to ensure there's space for the sentinel - * byte here. We expect a large proportion of allocations to be for sizes - * which are already a power of 2. If we were to always make space for a - * sentinel byte in MEMORY_CONTEXT_CHECKING builds, then we'd end up - * doubling the memory requirements for such allocations. + * Note that we always have space for the sentinel. To avoid wasting a lot + * of space - we expect a large proportion of allocations to be for sizes + * which are already a power of 2 - the space for the sentinel is added + * after the rounding to power of 2. */ fidx = AllocSetFreeIndex(size); chunk = set->freelist[fidx]; @@ -1060,9 +1063,11 @@ AllocSetAlloc(MemoryContext context, Size size, int flags) #ifdef MEMORY_CONTEXT_CHECKING chunk->requested_size = size; + /* set mark to catch clobber of "unused" space */ - if (size < GetChunkSizeFromFreeListIdx(fidx)) - set_sentinel(MemoryChunkGetPointer(chunk), size); + /* space for sentinel is added in addition to the freelist size */ + Assert(size <= GetChunkSizeFromFreeListIdx(fidx)); + set_sentinel(MemoryChunkGetPointer(chunk), size); #endif #ifdef RANDOMIZE_ALLOCATED_MEMORY /* fill the allocated space with junk */ @@ -1092,7 +1097,7 @@ AllocSetAlloc(MemoryContext context, Size size, int flags) * If there is enough room in the active allocation block, we will put the * chunk into that block. Else must start a new one. */ - if (unlikely(availspace < (chunk_size + ALLOC_CHUNKHDRSZ))) + if (unlikely(availspace < (chunk_size + ALLOC_CHUNKHDRSZ + MEMORY_CONTEXT_SENTINEL_SIZE))) return AllocSetAllocFromNewBlock(context, size, flags, fidx); /* There's enough space on the current block, so allocate from that */ @@ -1195,10 +1200,9 @@ AllocSetFree(void *pointer) elog(ERROR, "detected double pfree in %s %p", set->header.name, chunk); /* Test for someone scribbling on unused space in chunk */ - if (chunk->requested_size < GetChunkSizeFromFreeListIdx(fidx)) - if (!sentinel_ok(pointer, chunk->requested_size)) - elog(WARNING, "detected write past chunk end in %s %p", - set->header.name, chunk); + if (!sentinel_ok(pointer, chunk->requested_size)) + elog(WARNING, "detected write past chunk end in %s %p", + set->header.name, chunk); #endif #ifdef CLOBBER_FREED_MEMORY @@ -1275,18 +1279,13 @@ AllocSetRealloc(void *pointer, Size size, int flags) #ifdef MEMORY_CONTEXT_CHECKING /* Test for someone scribbling on unused space in chunk */ - Assert(chunk->requested_size < oldchksize); if (!sentinel_ok(pointer, chunk->requested_size)) elog(WARNING, "detected write past chunk end in %s %p", set->header.name, chunk); #endif -#ifdef MEMORY_CONTEXT_CHECKING - /* ensure there's always space for the sentinel byte */ - chksize = MAXALIGN(size + 1); -#else - chksize = MAXALIGN(size); -#endif + /* ensure there's space for the sentinel, if needed */ + chksize = MAXALIGN(size + MEMORY_CONTEXT_SENTINEL_SIZE); /* Do the realloc */ blksize = chksize + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ; @@ -1398,10 +1397,9 @@ AllocSetRealloc(void *pointer, Size size, int flags) elog(ERROR, "detected realloc of freed chunk in %s %p", set->header.name, chunk); /* Test for someone scribbling on unused space in chunk */ - if (chunk->requested_size < oldchksize) - if (!sentinel_ok(pointer, chunk->requested_size)) - elog(WARNING, "detected write past chunk end in %s %p", - set->header.name, chunk); + if (!sentinel_ok(pointer, chunk->requested_size)) + elog(WARNING, "detected write past chunk end in %s %p", + set->header.name, chunk); #endif /* @@ -1435,8 +1433,7 @@ AllocSetRealloc(void *pointer, Size size, int flags) oldchksize - size); /* set mark to catch clobber of "unused" space */ - if (size < oldchksize) - set_sentinel(pointer, size); + set_sentinel(pointer, size); #else /* !MEMORY_CONTEXT_CHECKING */ /* @@ -1752,6 +1749,13 @@ AllocSetCheck(MemoryContext context) chsize = GetChunkSizeFromFreeListIdx(fidx); /* aligned chunk size */ + /* + * Need to include the overhead for the sentinel in the + * calculation, otherwise the offset of the next chunk would + * be wrong. + */ + chsize += MEMORY_CONTEXT_SENTINEL_SIZE; + /* * Check the stored block offset correctly references this * block. diff --git a/src/backend/utils/mmgr/bump.c b/src/backend/utils/mmgr/bump.c index 9bb579935db..63decf89453 100644 --- a/src/backend/utils/mmgr/bump.c +++ b/src/backend/utils/mmgr/bump.c @@ -324,12 +324,8 @@ BumpAllocLarge(MemoryContext context, Size size, int flags) /* validate 'size' is within the limits for the given 'flags' */ MemoryContextCheckSize(context, size, flags); -#ifdef MEMORY_CONTEXT_CHECKING - /* ensure there's always space for the sentinel byte */ - chunk_size = MAXALIGN(size + 1); -#else - chunk_size = MAXALIGN(size); -#endif + /* ensure there's space for the sentinel, if needed */ + chunk_size = MAXALIGN(size + MEMORY_CONTEXT_SENTINEL_SIZE); required_size = chunk_size + Bump_CHUNKHDRSZ; blksize = required_size + Bump_BLOCKHDRSZ; @@ -357,7 +353,7 @@ BumpAllocLarge(MemoryContext context, Size size, int flags) chunk->requested_size = size; /* set mark to catch clobber of "unused" space */ - Assert(size < chunk_size); + Assert(size + MEMORY_CONTEXT_SENTINEL_SIZE <= chunk_size); set_sentinel(MemoryChunkGetPointer(chunk), size); #endif #ifdef RANDOMIZE_ALLOCATED_MEMORY @@ -421,7 +417,7 @@ BumpAllocChunkFromBlock(MemoryContext context, BumpBlock *block, Size size, MemoryChunkSetHdrMask(chunk, block, chunk_size, MCTX_BUMP_ID); chunk->requested_size = size; /* set mark to catch clobber of "unused" space */ - Assert(size < chunk_size); + Assert(size + MEMORY_CONTEXT_SENTINEL_SIZE <= chunk_size); set_sentinel(MemoryChunkGetPointer(chunk), size); #ifdef RANDOMIZE_ALLOCATED_MEMORY @@ -523,12 +519,8 @@ BumpAlloc(MemoryContext context, Size size, int flags) Assert(BumpIsValid(set)); -#ifdef MEMORY_CONTEXT_CHECKING - /* ensure there's always space for the sentinel byte */ - chunk_size = MAXALIGN(size + 1); -#else - chunk_size = MAXALIGN(size); -#endif + /* ensure there's space for the sentinel, if needed */ + chunk_size = MAXALIGN(size + MEMORY_CONTEXT_SENTINEL_SIZE); /* * If requested size exceeds maximum for chunks we hand the request off to diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c index 609c9bdc9a6..805f2cd34cc 100644 --- a/src/backend/utils/mmgr/generation.c +++ b/src/backend/utils/mmgr/generation.c @@ -372,12 +372,8 @@ GenerationAllocLarge(MemoryContext context, Size size, int flags) /* validate 'size' is within the limits for the given 'flags' */ MemoryContextCheckSize(context, size, flags); -#ifdef MEMORY_CONTEXT_CHECKING - /* ensure there's always space for the sentinel byte */ - chunk_size = MAXALIGN(size + 1); -#else - chunk_size = MAXALIGN(size); -#endif + /* ensure there's space for the sentinel, if needed */ + chunk_size = MAXALIGN(size + MEMORY_CONTEXT_SENTINEL_SIZE); required_size = chunk_size + Generation_CHUNKHDRSZ; blksize = required_size + Generation_BLOCKHDRSZ; @@ -407,7 +403,7 @@ GenerationAllocLarge(MemoryContext context, Size size, int flags) #ifdef MEMORY_CONTEXT_CHECKING chunk->requested_size = size; /* set mark to catch clobber of "unused" space */ - Assert(size < chunk_size); + Assert(size + MEMORY_CONTEXT_SENTINEL_SIZE <= chunk_size); set_sentinel(MemoryChunkGetPointer(chunk), size); #endif #ifdef RANDOMIZE_ALLOCATED_MEMORY @@ -559,12 +555,8 @@ GenerationAlloc(MemoryContext context, Size size, int flags) Assert(GenerationIsValid(set)); -#ifdef MEMORY_CONTEXT_CHECKING - /* ensure there's always space for the sentinel byte */ - chunk_size = MAXALIGN(size + 1); -#else - chunk_size = MAXALIGN(size); -#endif + /* ensure there's space for the sentinel, if needed */ + chunk_size = MAXALIGN(size + MEMORY_CONTEXT_SENTINEL_SIZE); /* * If requested size exceeds maximum for chunks we hand the request off to @@ -1207,7 +1199,7 @@ GenerationCheck(MemoryContext context) name, block, chunk); /* check sentinel */ - Assert(chunk->requested_size < chunksize); + Assert(chunk->requested_size + MEMORY_CONTEXT_SENTINEL_SIZE <= chunksize); if (!sentinel_ok(chunk, Generation_CHUNKHDRSZ + chunk->requested_size)) elog(WARNING, "problem in Generation %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 917cc0ac771..5284c365a32 100644 --- a/src/backend/utils/mmgr/mcxt.c +++ b/src/backend/utils/mmgr/mcxt.c @@ -1529,10 +1529,8 @@ MemoryContextAllocAligned(MemoryContext context, */ alloc_size = size + PallocAlignedExtraBytes(alignto); -#ifdef MEMORY_CONTEXT_CHECKING - /* ensure there's space for a sentinel byte */ - alloc_size += 1; -#endif + /* ensure there's space for the sentinel */ + alloc_size += MEMORY_CONTEXT_SENTINEL_SIZE; /* * Perform the actual allocation, but do not pass down MCXT_ALLOC_ZERO. diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c index 2ad325547fd..d5cae029cad 100644 --- a/src/backend/utils/mmgr/slab.c +++ b/src/backend/utils/mmgr/slab.c @@ -342,12 +342,8 @@ SlabContextCreate(MemoryContext parent, chunkSize = sizeof(MemoryChunk *); /* length of the maxaligned chunk including the chunk header */ -#ifdef MEMORY_CONTEXT_CHECKING - /* ensure there's always space for the sentinel byte */ - fullChunkSize = Slab_CHUNKHDRSZ + MAXALIGN(chunkSize + 1); -#else - fullChunkSize = Slab_CHUNKHDRSZ + MAXALIGN(chunkSize); -#endif + /* ensure there's space for the sentinel, if needed */ + fullChunkSize = Slab_CHUNKHDRSZ + MAXALIGN(chunkSize + MEMORY_CONTEXT_SENTINEL_SIZE); Assert(fullChunkSize <= MEMORYCHUNK_MAX_VALUE); @@ -541,7 +537,7 @@ SlabAllocSetupNewChunk(MemoryContext context, SlabBlock *block, #ifdef MEMORY_CONTEXT_CHECKING chunk->requested_size = size; /* slab mark to catch clobber of "unused" space */ - Assert(slab->chunkSize < (slab->fullChunkSize - Slab_CHUNKHDRSZ)); + Assert(slab->chunkSize + MEMORY_CONTEXT_SENTINEL_SIZE <= (slab->fullChunkSize - Slab_CHUNKHDRSZ)); set_sentinel(MemoryChunkGetPointer(chunk), size); VALGRIND_MAKE_MEM_NOACCESS(((char *) chunk) + Slab_CHUNKHDRSZ + slab->chunkSize, @@ -755,7 +751,7 @@ SlabFree(void *pointer) elog(ERROR, "detected double pfree in %s %p", slab->header.name, chunk); /* Test for someone scribbling on unused space in chunk */ - Assert(slab->chunkSize < (slab->fullChunkSize - Slab_CHUNKHDRSZ)); + Assert(slab->chunkSize + MEMORY_CONTEXT_SENTINEL_SIZE <= (slab->fullChunkSize - Slab_CHUNKHDRSZ)); if (!sentinel_ok(pointer, slab->chunkSize)) elog(WARNING, "detected write past chunk end in %s %p", slab->header.name, chunk); @@ -1165,8 +1161,8 @@ SlabCheck(MemoryContext context) elog(WARNING, "problem in slab %s: bogus block link in block %p, chunk %p", name, block, chunk); - /* check the sentinel byte is intact */ - Assert(slab->chunkSize < (slab->fullChunkSize - Slab_CHUNKHDRSZ)); + /* check the sentinel is intact */ + Assert(slab->chunkSize + MEMORY_CONTEXT_SENTINEL_SIZE <= (slab->fullChunkSize - Slab_CHUNKHDRSZ)); if (!sentinel_ok(chunk, Slab_CHUNKHDRSZ + slab->chunkSize)) elog(WARNING, "problem in slab %s: detected write past chunk end in block %p, chunk %p", name, block, chunk); base-commit: e01fc6315f19a794301ca8889d37ae629862e2ac -- 2.54.0.450.g9ac3f193c0