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 c477f3e44.  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

diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 026f5456760af2fe435d77bc620f3e4c38613976..2589941ec44ea2b8b2c6546186e8ea04580ec07c 100644 (file)
--- 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);
@@ -1187,21 +1187,29 @@ 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 randomize the extra space if we know the prior request.
+        * When using Valgrind, 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 marked any
+        * newly-allocated part (from oldchksize to chksize) UNDEFINED, but we
+        * also need to adjust trailing bytes from the old allocation (from
+        * chunk->requested_size to oldchksize) as they are marked NOACCESS.
+        * Make sure not to mark too many bytes in case chunk->requested_size
+        * < size < oldchksize.
         */
 #ifdef USE_VALGRIND
-       if (oldsize > chunk->requested_size)
+       if (Min(size, oldchksize) > chunk->requested_size)
            VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + chunk->requested_size,
-                                       oldsize - chunk->requested_size);
+                                       Min(size, oldchksize) - chunk->requested_size);
+#endif
 #endif
 
        chunk->requested_size = size;
@@ -1211,11 +1219,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 beyond the new
+        * allocation in case it's smaller than the old one.
         */
-       VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
+       VALGRIND_MAKE_MEM_DEFINED(pointer, Min(size, oldchksize));
 #endif
 
        /* Ensure any padding bytes are marked NOACCESS. */
@@ -1240,11 +1251,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);
@@ -1255,7 +1266,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;
@@ -1278,10 +1289,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 */
 
@@ -1290,7 +1301,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
 
@@ -1313,6 +1324,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);
@@ -1337,6 +1349,7 @@ AllocSetRealloc(void *pointer, Size size)
 #ifdef MEMORY_CONTEXT_CHECKING
        oldsize = chunk->requested_size;
 #else
+       oldsize = oldchksize;
        VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
 #endif