Fix explicit valgrind interaction in read_stream.c.
authorThomas Munro <tmunro@postgresql.org>
Sat, 15 Feb 2025 00:13:19 +0000 (13:13 +1300)
committerThomas Munro <tmunro@postgresql.org>
Fri, 21 Feb 2025 02:16:37 +0000 (15:16 +1300)
This is a back-patch of commits 2a8a0067 and 2509b857 into
REL_17_STABLE.  It's doesn't fix any known live bug in PostgreSQL v17
itself, but an extension could in theory have used the per-buffer data
feature and seen spurious errors under Valgrind.

Discussion: https://postgr.es/m/CA%2BhUKG%2Bg6aXpi2FEHqeLOzE%2BxYw%3DOV%2B-N5jhOEnnV%2BF0USM9xA%40mail.gmail.com

src/backend/storage/aio/read_stream.c

index 9b962c301bff620a297259c4fd580a9219271172..b5be92d4611283a76152ae9f99eab921e9a4974d 100644 (file)
@@ -176,9 +176,20 @@ read_stream_get_block(ReadStream *stream, void *per_buffer_data)
    if (blocknum != InvalidBlockNumber)
        stream->buffered_blocknum = InvalidBlockNumber;
    else
+   {
+       /*
+        * Tell Valgrind that the per-buffer data is undefined.  That replaces
+        * the "noaccess" state that was set when the consumer moved past this
+        * entry last time around the queue, and should also catch callbacks
+        * that fail to initialize data that the buffer consumer later
+        * accesses.  On the first go around, it is undefined already.
+        */
+       VALGRIND_MAKE_MEM_UNDEFINED(per_buffer_data,
+                                   stream->per_buffer_data_size);
        blocknum = stream->callback(stream,
                                    stream->callback_private_data,
                                    per_buffer_data);
+   }
 
    return blocknum;
 }
@@ -687,8 +698,11 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
    }
 
 #ifdef CLOBBER_FREED_MEMORY
-   /* Clobber old buffer and per-buffer data for debugging purposes. */
+   /* Clobber old buffer for debugging purposes. */
    stream->buffers[oldest_buffer_index] = InvalidBuffer;
+#endif
+
+#if defined(CLOBBER_FREED_MEMORY) || defined(USE_VALGRIND)
 
    /*
     * The caller will get access to the per-buffer data, until the next call.
@@ -697,11 +711,23 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
     * that is holding a dangling pointer to it.
     */
    if (stream->per_buffer_data)
-       wipe_mem(get_per_buffer_data(stream,
-                                    oldest_buffer_index == 0 ?
-                                    stream->queue_size - 1 :
-                                    oldest_buffer_index - 1),
-                stream->per_buffer_data_size);
+   {
+       void       *per_buffer_data;
+
+       per_buffer_data = get_per_buffer_data(stream,
+                                             oldest_buffer_index == 0 ?
+                                             stream->queue_size - 1 :
+                                             oldest_buffer_index - 1);
+
+#if defined(CLOBBER_FREED_MEMORY)
+       /* This also tells Valgrind the memory is "noaccess". */
+       wipe_mem(per_buffer_data, stream->per_buffer_data_size);
+#elif defined(USE_VALGRIND)
+       /* Tell it ourselves. */
+       VALGRIND_MAKE_MEM_NOACCESS(per_buffer_data,
+                                  stream->per_buffer_data_size);
+#endif
+   }
 #endif
 
    /* Pin transferred to caller. */