Skip to content

Commit 0e48038

Browse files
david-rowleydavid-rowley
committed
Make more effort to put a sentinel at the end of allocated memory
Traditionally, in MEMORY_CONTEXT_CHECKING builds, we only ever marked a sentinel byte just beyond the requested size if there happened to be enough space on the chunk to do so. For Slab and Generation context types, we only rounded the size of the chunk up to the next maxalign boundary, so it was often not that likely that those would ever have space for the sentinel given that the majority of allocation requests are going to be for sizes which are maxaligned. For AllocSet, it was a little different as smaller allocations are rounded up to the next power-of-2 value rather than the next maxalign boundary, so we're a bit more likely to have space for the sentinel byte, especially when we get away from tiny sized allocations such as 8 or 16 bytes. Here we make more of an effort to allow space so that there is enough room for the sentinel byte in more cases. This makes it more likely that we'll detect when buggy code accidentally writes beyond the end of any of its memory allocations. Each of the 3 MemoryContext types has been changed as follows: The Slab allocator will now always set a sentinel byte. Both the current usages of this MemoryContext type happen to use chunk sizes which were on the maxalign boundary, so these never used sentinel bytes previously. For the Generation allocator, we now always ensure there's enough space in the allocation for a sentinel byte. For AllocSet, this commit makes an adjustment for allocation sizes which are greater than allocChunkLimit. We now ensure there is always space for a sentinel byte. We don't alter the sentinel behavior for request sizes <= allocChunkLimit. Making way for the sentinel byte for power-of-2 request sizes would require doubling up to the next power of 2. Some analysis done on the request sizes made during installcheck shows that a fairly large portion of allocation requests are for power-of-2 sizes. The amount of additional memory for the sentinel there seems prohibitive, so we do nothing for those here. Author: David Rowley Discussion: https://postgr.es/m/3478405.1661824539@sss.pgh.pa.us
1 parent f98d074 commit 0e48038

File tree

3 files changed

+74
-47
lines changed

3 files changed

+74
-47
lines changed

‎src/backend/utils/mmgr/aset.c

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -705,7 +705,13 @@ AllocSetAlloc(MemoryContext context, Size size)
705705
*/
706706
if (size > set->allocChunkLimit)
707707
{
708+
#ifdef MEMORY_CONTEXT_CHECKING
709+
/* ensure there's always space for the sentinel byte */
710+
chunk_size = MAXALIGN(size + 1);
711+
#else
708712
chunk_size = MAXALIGN(size);
713+
#endif
714+
709715
blksize = chunk_size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ;
710716
block = (AllocBlock) malloc(blksize);
711717
if (block == NULL)
@@ -724,8 +730,8 @@ AllocSetAlloc(MemoryContext context, Size size)
724730
#ifdef MEMORY_CONTEXT_CHECKING
725731
chunk->requested_size = size;
726732
/* set mark to catch clobber of "unused" space */
727-
if (size < chunk_size)
728-
set_sentinel(MemoryChunkGetPointer(chunk), size);
733+
Assert(size < chunk_size);
734+
set_sentinel(MemoryChunkGetPointer(chunk), size);
729735
#endif
730736
#ifdef RANDOMIZE_ALLOCATED_MEMORY
731737
/* fill the allocated space with junk */
@@ -766,6 +772,12 @@ AllocSetAlloc(MemoryContext context, Size size)
766772
* corresponding free list to see if there is a free chunk we could reuse.
767773
* If one is found, remove it from the free list, make it again a member
768774
* of the alloc set and return its data address.
775+
*
776+
* Note that we don't attempt to ensure there's space for the sentinel
777+
* byte here. We expect a large proportion of allocations to be for sizes
778+
* which are already a power of 2. If we were to always make space for a
779+
* sentinel byte in MEMORY_CONTEXT_CHECKING builds, then we'd end up
780+
* doubling the memory requirements for such allocations.
769781
*/
770782
fidx = AllocSetFreeIndex(size);
771783
chunk = set->freelist[fidx];
@@ -992,10 +1004,10 @@ AllocSetFree(void *pointer)
9921004
Size chunk_size = block->endptr - (char *) pointer;
9931005

9941006
/* Test for someone scribbling on unused space in chunk */
995-
if (chunk->requested_size < chunk_size)
996-
if (!sentinel_ok(pointer, chunk->requested_size))
997-
elog(WARNING, "detected write past chunk end in %s %p",
998-
set->header.name, chunk);
1007+
Assert(chunk->requested_size < chunk_size);
1008+
if (!sentinel_ok(pointer, chunk->requested_size))
1009+
elog(WARNING, "detected write past chunk end in %s %p",
1010+
set->header.name, chunk);
9991011
}
10001012
#endif
10011013

@@ -1098,10 +1110,10 @@ AllocSetRealloc(void *pointer, Size size)
10981110

10991111
#ifdef MEMORY_CONTEXT_CHECKING
11001112
/* Test for someone scribbling on unused space in chunk */
1101-
if (chunk->requested_size < oldsize)
1102-
if (!sentinel_ok(pointer, chunk->requested_size))
1103-
elog(WARNING, "detected write past chunk end in %s %p",
1104-
set->header.name, chunk);
1113+
Assert(chunk->requested_size < oldsize);
1114+
if (!sentinel_ok(pointer, chunk->requested_size))
1115+
elog(WARNING, "detected write past chunk end in %s %p",
1116+
set->header.name, chunk);
11051117
#endif
11061118

11071119
/*
@@ -1111,7 +1123,12 @@ AllocSetRealloc(void *pointer, Size size)
11111123
if (block->freeptr != block->endptr)
11121124
elog(ERROR, "could not find block containing chunk %p", chunk);
11131125

1126+
#ifdef MEMORY_CONTEXT_CHECKING
1127+
/* ensure there's always space for the sentinel byte */
1128+
chksize = MAXALIGN(size + 1);
1129+
#else
11141130
chksize = MAXALIGN(size);
1131+
#endif
11151132

11161133
/* Do the realloc */
11171134
blksize = chksize + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ;
@@ -1162,8 +1179,8 @@ AllocSetRealloc(void *pointer, Size size)
11621179

11631180
chunk->requested_size = size;
11641181
/* set mark to catch clobber of "unused" space */
1165-
if (size < chksize)
1166-
set_sentinel(pointer, size);
1182+
Assert(size < chksize);
1183+
set_sentinel(pointer, size);
11671184
#else /* !MEMORY_CONTEXT_CHECKING */
11681185

11691186
/*

‎src/backend/utils/mmgr/generation.c

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -342,8 +342,16 @@ GenerationAlloc(MemoryContext context, Size size)
342342
GenerationContext *set = (GenerationContext *) context;
343343
GenerationBlock *block;
344344
MemoryChunk *chunk;
345-
Size chunk_size = MAXALIGN(size);
346-
Size required_size = chunk_size + Generation_CHUNKHDRSZ;
345+
Size chunk_size;
346+
Size required_size;
347+
348+
#ifdef MEMORY_CONTEXT_CHECKING
349+
/* ensure there's always space for the sentinel byte */
350+
chunk_size = MAXALIGN(size + 1);
351+
#else
352+
chunk_size = MAXALIGN(size);
353+
#endif
354+
required_size = chunk_size + Generation_CHUNKHDRSZ;
347355

348356
/* is it an over-sized chunk? if yes, allocate special block */
349357
if (chunk_size > set->allocChunkLimit)
@@ -373,8 +381,8 @@ GenerationAlloc(MemoryContext context, Size size)
373381
#ifdef MEMORY_CONTEXT_CHECKING
374382
chunk->requested_size = size;
375383
/* set mark to catch clobber of "unused" space */
376-
if (size < chunk_size)
377-
set_sentinel(MemoryChunkGetPointer(chunk), size);
384+
Assert(size < chunk_size);
385+
set_sentinel(MemoryChunkGetPointer(chunk), size);
378386
#endif
379387
#ifdef RANDOMIZE_ALLOCATED_MEMORY
380388
/* fill the allocated space with junk */
@@ -491,8 +499,8 @@ GenerationAlloc(MemoryContext context, Size size)
491499
#ifdef MEMORY_CONTEXT_CHECKING
492500
chunk->requested_size = size;
493501
/* set mark to catch clobber of "unused" space */
494-
if (size < chunk_size)
495-
set_sentinel(MemoryChunkGetPointer(chunk), size);
502+
Assert(size < chunk_size);
503+
set_sentinel(MemoryChunkGetPointer(chunk), size);
496504
#endif
497505
#ifdef RANDOMIZE_ALLOCATED_MEMORY
498506
/* fill the allocated space with junk */
@@ -634,10 +642,10 @@ GenerationFree(void *pointer)
634642

635643
#ifdef MEMORY_CONTEXT_CHECKING
636644
/* Test for someone scribbling on unused space in chunk */
637-
if (chunk->requested_size < chunksize)
638-
if (!sentinel_ok(pointer, chunk->requested_size))
639-
elog(WARNING, "detected write past chunk end in %s %p",
640-
((MemoryContext) block->context)->name, chunk);
645+
Assert(chunk->requested_size < chunksize);
646+
if (!sentinel_ok(pointer, chunk->requested_size))
647+
elog(WARNING, "detected write past chunk end in %s %p",
648+
((MemoryContext) block->context)->name, chunk);
641649
#endif
642650

643651
#ifdef CLOBBER_FREED_MEMORY
@@ -727,10 +735,10 @@ GenerationRealloc(void *pointer, Size size)
727735

728736
#ifdef MEMORY_CONTEXT_CHECKING
729737
/* Test for someone scribbling on unused space in chunk */
730-
if (chunk->requested_size < oldsize)
731-
if (!sentinel_ok(pointer, chunk->requested_size))
732-
elog(WARNING, "detected write past chunk end in %s %p",
733-
((MemoryContext) set)->name, chunk);
738+
Assert(chunk->requested_size < oldsize);
739+
if (!sentinel_ok(pointer, chunk->requested_size))
740+
elog(WARNING, "detected write past chunk end in %s %p",
741+
((MemoryContext) set)->name, chunk);
734742
#endif
735743

736744
/*
@@ -769,8 +777,7 @@ GenerationRealloc(void *pointer, Size size)
769777
oldsize - size);
770778

771779
/* set mark to catch clobber of "unused" space */
772-
if (size < oldsize)
773-
set_sentinel(pointer, size);
780+
set_sentinel(pointer, size);
774781
#else /* !MEMORY_CONTEXT_CHECKING */
775782

776783
/*
@@ -1034,8 +1041,8 @@ GenerationCheck(MemoryContext context)
10341041
name, block, chunk);
10351042

10361043
/* check sentinel */
1037-
if (chunk->requested_size < chunksize &&
1038-
!sentinel_ok(chunk, Generation_CHUNKHDRSZ + chunk->requested_size))
1044+
Assert(chunk->requested_size < chunksize);
1045+
if (!sentinel_ok(chunk, Generation_CHUNKHDRSZ + chunk->requested_size))
10391046
elog(WARNING, "problem in Generation %s: detected write past chunk end in block %p, chunk %p",
10401047
name, block, chunk);
10411048
}

‎src/backend/utils/mmgr/slab.c

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,12 @@ SlabContextCreate(MemoryContext parent,
145145
chunkSize = sizeof(int);
146146

147147
/* chunk, including SLAB header (both addresses nicely aligned) */
148+
#ifdef MEMORY_CONTEXT_CHECKING
149+
/* ensure there's always space for the sentinel byte */
150+
fullChunkSize = Slab_CHUNKHDRSZ + MAXALIGN(chunkSize + 1);
151+
#else
148152
fullChunkSize = Slab_CHUNKHDRSZ + MAXALIGN(chunkSize);
153+
#endif
149154

150155
/* Make sure the block can store at least one chunk. */
151156
if (blockSize < fullChunkSize + Slab_BLOCKHDRSZ)
@@ -420,14 +425,12 @@ SlabAlloc(MemoryContext context, Size size)
420425
MCTX_SLAB_ID);
421426
#ifdef MEMORY_CONTEXT_CHECKING
422427
/* slab mark to catch clobber of "unused" space */
423-
if (slab->chunkSize < (slab->fullChunkSize - Slab_CHUNKHDRSZ))
424-
{
425-
set_sentinel(MemoryChunkGetPointer(chunk), size);
426-
VALGRIND_MAKE_MEM_NOACCESS(((char *) chunk) +
427-
Slab_CHUNKHDRSZ + slab->chunkSize,
428-
slab->fullChunkSize -
429-
(slab->chunkSize + Slab_CHUNKHDRSZ));
430-
}
428+
Assert(slab->chunkSize < (slab->fullChunkSize - Slab_CHUNKHDRSZ));
429+
set_sentinel(MemoryChunkGetPointer(chunk), size);
430+
VALGRIND_MAKE_MEM_NOACCESS(((char *) chunk) +
431+
Slab_CHUNKHDRSZ + slab->chunkSize,
432+
slab->fullChunkSize -
433+
(slab->chunkSize + Slab_CHUNKHDRSZ));
431434
#endif
432435

433436
#ifdef RANDOMIZE_ALLOCATED_MEMORY
@@ -454,10 +457,10 @@ SlabFree(void *pointer)
454457

455458
#ifdef MEMORY_CONTEXT_CHECKING
456459
/* Test for someone scribbling on unused space in chunk */
457-
if (slab->chunkSize < (slab->fullChunkSize - Slab_CHUNKHDRSZ))
458-
if (!sentinel_ok(pointer, slab->chunkSize))
459-
elog(WARNING, "detected write past chunk end in %s %p",
460-
slab->header.name, chunk);
460+
Assert(slab->chunkSize < (slab->fullChunkSize - Slab_CHUNKHDRSZ));
461+
if (!sentinel_ok(pointer, slab->chunkSize))
462+
elog(WARNING, "detected write past chunk end in %s %p",
463+
slab->header.name, chunk);
461464
#endif
462465

463466
/* compute index of the chunk with respect to block start */
@@ -744,11 +747,11 @@ SlabCheck(MemoryContext context)
744747
elog(WARNING, "problem in slab %s: bogus block link in block %p, chunk %p",
745748
name, block, chunk);
746749

747-
/* there might be sentinel (thanks to alignment) */
748-
if (slab->chunkSize < (slab->fullChunkSize - Slab_CHUNKHDRSZ))
749-
if (!sentinel_ok(chunk, Slab_CHUNKHDRSZ + slab->chunkSize))
750-
elog(WARNING, "problem in slab %s: detected write past chunk end in block %p, chunk %p",
751-
name, block, chunk);
750+
/* check the sentinel byte is intact */
751+
Assert(slab->chunkSize < (slab->fullChunkSize - Slab_CHUNKHDRSZ));
752+
if (!sentinel_ok(chunk, Slab_CHUNKHDRSZ + slab->chunkSize))
753+
elog(WARNING, "problem in slab %s: detected write past chunk end in block %p, chunk %p",
754+
name, block, chunk);
752755
}
753756
}
754757

0 commit comments

Comments
 (0)