postgres.git / summary / log / commit / refs

commit    2a8a00674e973dec4d7d74d5e16c7d7cdedf2be5
Author:   Thomas Munro <tmunro@postgresql.org>
Date:     Sat Feb 15 00:13:19 2025 +0000
First released in: 18.0, 17.5

    Fix explicit valgrind interaction in read_stream.c.
    
    By calling wipe_mem() on per-buffer data memory that has been released,
    we are also telling Valgrind that the memory is "noaccess".  We need to
    set it to "undefined" before giving it to the registered callback to
    fill in, when a slot is reused.
    
    As discovered by build farm animal skink when the VACUUM streamification
    patches landed (the first users of per-buffer data).
    
    Pushing to master only for now, to clear the error on skink.  It's also
    possible that external code might discover the per-buffer data feature
    in v17, and reasonable to expect Valgrind not to produce spurious
    memcheck reports, but the back-patch is deferred until after the
    imminent minor release is out of the way.
    
    Reviewed-by: Melanie Plageman <melanieplageman@gmail.com>
    Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
    Tested-by: Melanie Plageman <melanieplageman@gmail.com>
    Discussion: https://postgr.es/m/CA%2BhUKG%2Bg6aXpi2FEHqeLOzE%2BxYw%3DOV%2B-N5jhOEnnV%2BF0USM9xA%40mail.gmail.com


 src/backend/storage/aio/read_stream.c | 38 +++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index e4414b2e915..4b499dfb441 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -193,9 +193,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;
 }
@@ -752,8 +763,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.
@@ -762,11 +776,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_NO_ACCESS(per_buffer_data,
+									stream->per_buffer_data_size);
+#endif
+	}
 #endif
 
 	/* Pin transferred to caller. */


[parent: efdadeb2238f]