On Sat, Feb 15, 2025 at 10:50 AM Thomas Munro <thomas.mu...@gmail.com> wrote: > On Sat, Feb 15, 2025 at 7:30 AM Melanie Plageman > <melanieplage...@gmail.com> wrote: > > Seems valgrind doesn't like this [1]. I'm looking into it. > > Melanie was able to reproduce this on her local valgrind and > eventually we figured out that it's my fault. I put code into > read_stream.c that calls wipe_mem(), thinking that that was our > standard way of scribbling 0x7f on memory that you shouldn't access > again until it's reused. I didn't realise that wipe_mem() also tells > valgrind that the memory is now "no access". That makes sense for > palloc/pfree because when that range is allocated again it'll clear > that. The point is to help people discover that they have a dangling > reference to per-buffer data after they advance to the next buffer, > which wouldn't work because it's in a circular queue and could be > overwritten any time after that.
Here's a patch. Is there a tidier way to write this? It should probably be back-patched to 17, because external code might use per-buffer data (obviously v17 core doesn't or skink would have told us this sooner). It's not a good time to push to 17 today, though. Push to master now to cheer skink up and 17 some time later when the coast is clear, or just wait?
From bf4ba8b334c7ea6fcd33e8e6a6a4628c88161624 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Sat, 15 Feb 2025 11:06:30 +1300 Subject: [PATCH] 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). Back-patch to 17, where read streams landed. There aren't any users of per-buffer data in 17, but extension code might do that. Reported-by: Melanie Plageman <melanieplage...@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 | 39 ++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index e4414b2e915..d722eda7d8e 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,24 @@ 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(get_per_buffer_data(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. */ -- 2.48.1
