Hi hackers,
In 82d0a46ea32 AllocSetRealloc() was changed to allow decreasing size of
external chunks and give memory back to the malloc pool. Two
VALGRIND_MAKE_MEM_UNDEFINED() calls were not changed to work properly in the
case of decreasing size: they can mark memory behind the new allocated
memory
UNDEFINED. If this memory was already allocated and initialized, it's
expected
to be DEFINED. So it can cause false valgrind error reports. I fixed it in
0001
patch.
Also, it took me a while to understand what's going on there, so in 0002
patch
I tried to improve comments and renamed a variable. Its name "oldsize"
confused
me. I first thought "oldsize" and "size" represent the same parameters of
the
old and new chunk. But actually "size" is new "chunk->requested_size" and
"oldsize" is old "chksize". So I believe it's better to rename "oldsize"
into
"oldchksize".
Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
From 9556b7918e6abccb2dc19f20dbf572d41cd35cb4 Mon Sep 17 00:00:00 2001
From: Karina Litskevich <litskevichkar...@gmail.com>
Date: Tue, 14 Feb 2023 17:13:17 +0300
Subject: [PATCH v1 1/2] Fix VALGRIND_MAKE_MEM_DEFINED() calls
---
src/backend/utils/mmgr/aset.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 740729b5d0..1ff0bb83cb 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -1199,7 +1199,7 @@ AllocSetRealloc(void *pointer, Size size)
* old allocation.
*/
#ifdef USE_VALGRIND
- if (oldsize > chunk->requested_size)
+ if (size > chunk->requested_size && oldsize > chunk->requested_size)
VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + chunk->requested_size,
oldsize - chunk->requested_size);
#endif
@@ -1215,7 +1215,10 @@ AllocSetRealloc(void *pointer, Size size)
* allocation; it could have been as small as one byte. We have to be
* conservative and just mark the entire old portion DEFINED.
*/
- VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
+ if (size >= oldsize)
+ VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
+ else
+ VALGRIND_MAKE_MEM_DEFINED(pointer, size);
#endif
/* Ensure any padding bytes are marked NOACCESS. */
--
2.25.1
From 7cdaee9caaf96564237e4cfa8279699a36942624 Mon Sep 17 00:00:00 2001
From: Karina Litskevich <litskevichkar...@gmail.com>
Date: Tue, 14 Feb 2023 17:18:30 +0300
Subject: [PATCH v1 2/2] Better comments and variable name in AllocSetRealloc()
---
src/backend/utils/mmgr/aset.c | 46 ++++++++++++++++++++---------------
1 file changed, 27 insertions(+), 19 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 1ff0bb83cb..35007750a9 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -1112,7 +1112,7 @@ AllocSetRealloc(void *pointer, Size size)
AllocBlock block;
AllocSet set;
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
- Size oldsize;
+ Size oldchksize;
int fidx;
/* Allow access to private part of chunk header. */
@@ -1140,11 +1140,11 @@ AllocSetRealloc(void *pointer, Size size)
set = block->aset;
- oldsize = block->endptr - (char *) pointer;
+ oldchksize = block->endptr - (char *) pointer;
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
- Assert(chunk->requested_size < oldsize);
+ 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);
@@ -1194,14 +1194,17 @@ AllocSetRealloc(void *pointer, Size size)
#endif
/*
- * realloc() (or randomize_mem()) will have left any newly-allocated
- * part UNDEFINED, but we may need to adjust trailing bytes from the
- * old allocation.
+ * If this is an increase, realloc() (or randomize_mem()) will have left
+ * any newly-allocated part UNDEFINED, but we may need to adjust
+ * trailing bytes from the old allocation as they are marked NOACCESS.
*/
#ifdef USE_VALGRIND
- if (size > chunk->requested_size && oldsize > chunk->requested_size)
+ if (size > chunk->requested_size && oldchksize > chunk->requested_size)
+ {
+ Assert(chksize >= oldchksize);
VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + chunk->requested_size,
- oldsize - chunk->requested_size);
+ oldchksize - chunk->requested_size);
+ }
#endif
chunk->requested_size = size;
@@ -1211,12 +1214,15 @@ AllocSetRealloc(void *pointer, Size size)
#else /* !MEMORY_CONTEXT_CHECKING */
/*
- * We don't know how much of the old chunk size was the actual
- * allocation; it could have been as small as one byte. We have to be
- * conservative and just mark the entire old portion DEFINED.
+ * We may need to adjust marking of bytes from the old allocation as
+ * some of them may be marked NOACCESS. We don't know how much of the
+ * old chunk size was the requested size; it could have been as small as
+ * one byte. We have to be conservative and just mark the entire old
+ * portion DEFINED. Make sure not to mark memory behind the new
+ * allocation in case it's smaller than old one.
*/
- if (size >= oldsize)
- VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
+ if (size >= oldchksize)
+ VALGRIND_MAKE_MEM_DEFINED(pointer, oldchksize);
else
VALGRIND_MAKE_MEM_DEFINED(pointer, size);
#endif
@@ -1243,11 +1249,11 @@ AllocSetRealloc(void *pointer, Size size)
fidx = MemoryChunkGetValue(chunk);
Assert(FreeListIdxIsValid(fidx));
- oldsize = GetChunkSizeFromFreeListIdx(fidx);
+ oldchksize = GetChunkSizeFromFreeListIdx(fidx);
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
- if (chunk->requested_size < oldsize)
+ 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);
@@ -1258,7 +1264,7 @@ AllocSetRealloc(void *pointer, Size size)
* allocated area already is >= the new size. (In particular, we will
* fall out here if the requested size is a decrease.)
*/
- if (oldsize >= size)
+ if (oldchksize >= size)
{
#ifdef MEMORY_CONTEXT_CHECKING
Size oldrequest = chunk->requested_size;
@@ -1281,10 +1287,10 @@ AllocSetRealloc(void *pointer, Size size)
size - oldrequest);
else
VALGRIND_MAKE_MEM_NOACCESS((char *) pointer + size,
- oldsize - size);
+ oldchksize - size);
/* set mark to catch clobber of "unused" space */
- if (size < oldsize)
+ if (size < oldchksize)
set_sentinel(pointer, size);
#else /* !MEMORY_CONTEXT_CHECKING */
@@ -1293,7 +1299,7 @@ AllocSetRealloc(void *pointer, Size size)
* the old request or shrinking it, so we conservatively mark the
* entire new allocation DEFINED.
*/
- VALGRIND_MAKE_MEM_NOACCESS(pointer, oldsize);
+ VALGRIND_MAKE_MEM_NOACCESS(pointer, oldchksize);
VALGRIND_MAKE_MEM_DEFINED(pointer, size);
#endif
@@ -1316,6 +1322,7 @@ AllocSetRealloc(void *pointer, Size size)
* memory indefinitely. See pgsql-hackers archives for 2007-08-11.)
*/
AllocPointer newPointer;
+ Size oldsize;
/* allocate new chunk */
newPointer = AllocSetAlloc((MemoryContext) set, size);
@@ -1340,6 +1347,7 @@ AllocSetRealloc(void *pointer, Size size)
#ifdef MEMORY_CONTEXT_CHECKING
oldsize = chunk->requested_size;
#else
+ oldsize = oldchksize;
VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
#endif
--
2.25.1
