8000 Fix erroneous Valgrind markings in AllocSetRealloc. · postgres/postgres@21bd818 · GitHub
[go: up one dir, main page]
Skip to content

Commit 21bd818

Browse files
tglsfdctglsfdc
committed
Fix erroneous Valgrind markings in AllocSetRealloc.
If asked to decrease the size of a large (>8K) palloc chunk, AllocSetRealloc could improperly change the Valgrind state of memory beyond the new end of the chunk: it would mark data UNDEFINED as far as the old end of the chunk after having done the realloc(3) call, thus tromping on the state of memory that no longer belongs to it. One would normally expect that memory to now be marked NOACCESS, so that this mislabeling might prevent detection of later errors. If realloc() had chosen to move the chunk someplace else (unlikely, but well within its rights) we could also mismark perfectly-valid DEFINED data as UNDEFINED, causing false-positive valgrind reports later. Also, any malloc bookkeeping data placed within this area might now be wrongly marked, causing additional problems. Fix by replacing relevant uses of "oldsize" with "Min(size, oldsize)". It's sufficient to mark as far as "size" when that's smaller, because whatever remains in the new chunk size will be marked NOACCESS below, and we expect realloc() to have taken care of marking the memory beyond the new official end of the chunk. While we're here, also rename the function's "oldsize" variable to "oldchksize" to more clearly explain what it actually holds, namely the distance to the end of the chunk (that is, requested size plus trailing padding). This is more consistent with the use of "size" and "chksize" to hold the new requested size and chunk size. Add a new variable "oldsize" in the one stanza where we're actually talking about the old requested size. Oversight in commit c477f3e. Back-patch to all supported branches, as that was, just in case anybody wants to do valgrind testing on back branches. Karina Litskevich Discussion: https://postgr.es/m/CACiT8iaAET-fmzjjZLjaJC4zwSJmrFyL7LAdHwaYyjjQOQ4hcg@mail.gmail.com
1 parent df931e9 commit 21bd818

File tree

1 file changed

+33
-20
lines changed

1 file changed

+33
-20
lines changed

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

Lines changed: 33 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1069,22 +1069,22 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
10691069
{
10701070
AllocSet set = (AllocSet) context;
10711071
AllocChunk chunk = AllocPointerGetChunk(pointer);
1072-
Size oldsize;
1072+
Size oldchksize;
10731073

10741074
/* Allow access to private part of chunk header. */
10751075
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
10761076

1077-
oldsize = chunk->size;
1077+
oldchksize = chunk->size;
10781078

10791079
#ifdef MEMORY_CONTEXT_CHECKING
10801080
/* Test for someone scribbling on unused space in chunk */
1081-
if (chunk->requested_size < oldsize)
1081+
if (chunk->requested_size < oldchksize)
10821082
if (!sentinel_ok(pointer, chunk->requested_size))
10831083
elog(WARNING, "detected write past chunk end in %s %p",
10841084
set->header.name, chunk);
10851085
#endif
10861086

1087-
if (oldsize > set->allocChunkLimit)
1087+
if (oldchksize > set->allocChunkLimit)
10881088
{
10891089
/*
10901090
* The chunk must have been allocated as a single-chunk block. Use
@@ -1103,7 +1103,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
11031103
if (block->aset != set ||
11041104
block->freeptr != block->endptr ||
11051105
block->freeptr != ((char *) block) +
1106-
(oldsize + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
1106+
(oldchksize + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
11071107
elog(ERROR, "could not find block containing chunk %p", chunk);
11081108

11091109
/*
@@ -1139,21 +1139,29 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
11391139

11401140
#ifdef MEMORY_CONTEXT_CHECKING
11411141
#ifdef RANDOMIZE_ALLOCATED_MEMORY
1142-
/* We can only fill the extra space if we know the prior request */
1142+
1143+
/*
1144+
* We can only randomize the extra space if we know the prior request.
1145+
* When using Valgrind, randomize_mem() also marks memory UNDE 8000 FINED.
1146+
*/
11431147
if (size > chunk->requested_size)
11441148
randomize_mem((char *) pointer + chunk->requested_size,
11451149
size - chunk->requested_size);
1146-
#endif
1150+
#else
11471151

11481152
/*
1149-
* realloc() (or randomize_mem()) will have left any newly-allocated
1150-
* part UNDEFINED, but we may need to adjust trailing bytes from the
1151-
* old allocation.
1153+
* If this is an increase, realloc() will have marked any
1154+
* newly-allocated part (from oldchksize to chksize) UNDEFINED, but we
1155+
* also need to adjust trailing bytes from the old allocation (from
1156+
* chunk->requested_size to oldchksize) as they are marked NOACCESS.
1157+
* Make sure not to mark too many bytes in case chunk->requested_size
1158+
* < size < oldchksize.
11521159
*/
11531160
#ifdef USE_VALGRIND
1154-
if (oldsize > chunk->requested_size)
1161+
if (Min(size, oldchksize) > chunk->requested_size)
11551162
VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + chunk->requested_size,
1156-
oldsize - chunk->requested_size);
1163+
Min(size, oldchksize) - chunk->requested_size);
1164+
#endif
11571165
#endif
11581166

11591167
chunk->requested_size = size;
@@ -1164,11 +1172,14 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
11641172
#else /* !MEMORY_CONTEXT_CHECKING */
11651173

11661174
/*
1167-
* We don't know how much of the old chunk size was the actual
1168-
* allocation; it could have been as small as one byte. We have to be
1169-
* conservative and just mark the entire old portion DEFINED.
1175+
* We may need to adjust marking of bytes from the old allocation as
1176+
* some of them may be marked NOACCESS. We don't know how much of the
1177+
* old chunk size was the requested size; it could have been as small
1178+
* as one byte. We have to be conservative and just mark the entire
1179+
* old portion DEFINED. Make sure not to mark memory beyond the new
1180+
* allocation in case it's smaller than the old one.
11701181
*/
1171-
VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
1182+
VALGRIND_MAKE_MEM_DEFINED(pointer, Min(size, oldchksize));
11721183
#endif
11731184

11741185
/* Ensure any padding bytes are marked NOACCESS. */
@@ -1185,7 +1196,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
11851196
* allocated area already is >= the new size. (In particular, we will
11861197
* fall out here if the requested size is a decrease.)
11871198
*/
1188-
else if (oldsize >= size)
1199+
else if (oldchksize >= size)
11891200
{
11901201
#ifdef MEMORY_CONTEXT_CHECKING
11911202
Size oldrequest = chunk->requested_size;
@@ -1208,10 +1219,10 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
12081219
size - oldrequest);
12091220
else
12101221
VALGRIND_MAKE_MEM_NOACCESS((char *) pointer + size,
1211-
oldsize - size);
1222+
oldchksize - size);
12121223

12131224
/* set mark to catch clobber of "unused" space */
1214-
if (size < oldsize)
1225+
if (size < oldchksize)
12151226
set_sentinel(pointer, size);
12161227
#else /* !MEMORY_CONTEXT_CHECKING */
12171228

@@ -1220,7 +1231,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
12201231
* the old request or shrinking it, so we conservatively mark the
12211232
* entire new allocation DEFINED.
12221233
*/
1223-
VALGRIND_MAKE_MEM_NOACCESS(pointer, oldsize);
1234+
VALGRIND_MAKE_MEM_NOACCESS(pointer, oldchksize);
12241235
VALGRIND_MAKE_MEM_DEFINED(pointer, size);
12251236
#endif
12261237

@@ -1243,6 +1254,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
12431254
* memory indefinitely. See pgsql-hackers archives for 2007-08-11.)
12441255
*/
12451256
AllocPointer newPointer;
1257+
Size oldsize;
12461258

12471259
/* allocate new chunk */
12481260
newPointer = AllocSetAlloc((MemoryContext) set, size);
@@ -1267,6 +1279,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
12671279
#ifdef MEMORY_CONTEXT_CHECKING
12681280
oldsize = chunk->requested_size;
12691281
#else
1282+
oldsize = oldchksize;
12701283
VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
12711284
#endif
12721285

0 commit comments

Comments
 (0)
0