Thank you, I moved comment changes to 0001 and rewrote the fix through Min().
> The first hunk in 0001 doesn't seem quite right yet:
>
> * 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
>
> If size < oldsize, aren't we still doing the wrong thing? Seems like
> maybe it has to be like
If size > chunk->requested_size than chksize >= oldsize and so we can mark this
memory without worries. Region from size to chksize will be marked NOACCESS
later anyway:
/* Ensure any padding bytes are marked NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS((char *) pointer + size, chksize - size);
I agree that it's not obvious, so I changed the first hunk like this:
- if (oldsize > chunk->requested_size)
+ if (Min(size, oldsize) > chunk->requested_size)
VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + chunk->requested_size,
- oldsize - chunk->requested_size);
+ Min(size, oldsize) - chunk->requested_size);
Any ideas on how to make this place easier to understand and comment above it
concise and clear are welcome.
There is another thing about this version. New line
+ Min(size, oldsize) - chunk->requested_size);
is longer than 80 symbols and I don't know what's the best way to avoid this
without making it look weird.
I also noticed that if RANDOMIZE_ALLOCATED_MEMORY is defined then
randomize_mem()
have already marked this memory UNDEFINED. So we only "may need to adjust
trailing bytes" if RANDOMIZE_ALLOCATED_MEMORY isn't defined. I reflected it in
v2 of 0001 too.
From 009ef9bbabcd71e0d5f2b60799c0b71d21fb9767 Mon Sep 17 00:00:00 2001
From: Karina Litskevich <litskevichkar...@gmail.com>
Date: Fri, 17 Feb 2023 15:32:05 +0300
Subject: [PATCH v2 2/2] Change variable name in AllocSetRealloc()
---
src/backend/utils/mmgr/aset.c | 32 +++++++++++++++++---------------
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ace4907ce9..9584d50b14 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);
@@ -1197,15 +1197,15 @@ AllocSetRealloc(void *pointer, Size size)
#else
/*
* If this is an increase, realloc() will have left any newly-allocated
- * part (from oldsize to chksize) UNDEFINED, but we may need to adjust
+ * part (from oldchksize to chksize) UNDEFINED, but we may need to adjust
* trailing bytes from the old allocation (from chunk->requested_size to
- * oldsize) as they are marked NOACCESS. Make sure not to mark extra
- * bytes in case chunk->requested_size < size < oldsize.
+ * oldchksize) as they are marked NOACCESS. Make sure not to mark extra
+ * bytes in case chunk->requested_size < size < oldchksize.
*/
#ifdef USE_VALGRIND
- if (Min(size, oldsize) > chunk->requested_size)
+ if (Min(size, oldchksize) > chunk->requested_size)
VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + chunk->requested_size,
- Min(size, oldsize) - chunk->requested_size);
+ Min(size, oldchksize) - chunk->requested_size);
#endif
#endif
@@ -1223,7 +1223,7 @@ AllocSetRealloc(void *pointer, Size size)
* portion DEFINED. Make sure not to mark memory behind the new
* allocation in case it's smaller than old one.
*/
- VALGRIND_MAKE_MEM_DEFINED(pointer, Min(size, oldsize));
+ VALGRIND_MAKE_MEM_DEFINED(pointer, Min(size, oldchksize));
#endif
/* Ensure any padding bytes are marked NOACCESS. */
@@ -1248,11 +1248,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);
@@ -1263,7 +1263,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;
@@ -1286,10 +1286,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 */
@@ -1298,7 +1298,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
@@ -1321,6 +1321,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);
@@ -1345,6 +1346,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
From 1c298fd3dc7894fa0718765a161fd4617e6df986 Mon Sep 17 00:00:00 2001
From: Karina Litskevich <litskevichkar...@gmail.com>
Date: Tue, 14 Feb 2023 17:13:17 +0300
Subject: [PATCH v2 1/2] Fix VALGRIND_MAKE_MEM_DEFINED() calls
---
src/backend/utils/mmgr/aset.c | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 740729b5d0..ace4907ce9 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -1187,21 +1187,26 @@ AllocSetRealloc(void *pointer, Size size)
#ifdef MEMORY_CONTEXT_CHECKING
#ifdef RANDOMIZE_ALLOCATED_MEMORY
- /* We can only fill the extra space if we know the prior request */
+ /*
+ * We can only fill the extra space if we know the prior request.
+ * Note: randomize_mem() also marks memory UNDEFINED.
+ */
if (size > chunk->requested_size)
randomize_mem((char *) pointer + chunk->requested_size,
size - chunk->requested_size);
-#endif
-
+#else
/*
- * 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() will have left any newly-allocated
+ * part (from oldsize to chksize) UNDEFINED, but we may need to adjust
+ * trailing bytes from the old allocation (from chunk->requested_size to
+ * oldsize) as they are marked NOACCESS. Make sure not to mark extra
+ * bytes in case chunk->requested_size < size < oldsize.
*/
#ifdef USE_VALGRIND
- if (oldsize > chunk->requested_size)
+ if (Min(size, oldsize) > chunk->requested_size)
VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + chunk->requested_size,
- oldsize - chunk->requested_size);
+ Min(size, oldsize) - chunk->requested_size);
+#endif
#endif
chunk->requested_size = size;
@@ -1211,11 +1216,14 @@ 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.
*/
- VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
+ VALGRIND_MAKE_MEM_DEFINED(pointer, Min(size, oldsize));
#endif
/* Ensure any padding bytes are marked NOACCESS. */
--
2.25.1
