Re: Confine vacuum skip logic to lazy_scan_skip

Hi,

On 2025-02-27 14:32:28 -0300, Ranier Vilela wrote:
> Hi.
>
> Em ter., 18 de fev. de 2025 às 11:31, Melanie Plageman <
> melanieplageman(at)gmail(dot)com> escreveu:
>
> > On Sun, Feb 16, 2025 at 1:12 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > >
> > > Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
> > > > Thanks! It's green again.
> > >
> > > The security team's Coverity instance complained about this patch:
> > >
> > > *** CID 1642971: Null pointer dereferences (FORWARD_NULL)
> > >
> > /srv/coverity/git/pgsql-git/postgresql/src/backend/access/heap/vacuumlazy.c:
> > 1295 in lazy_scan_heap()
> > > 1289 buf = read_stream_next_buffer(stream,
> > &per_buffer_data);
> > > 1290
> > > 1291 /* The relation is exhausted. */
> > > 1292 if (!BufferIsValid(buf))
> > > 1293 break;
> > > 1294
> > > >>> CID 1642971: Null pointer dereferences (FORWARD_NULL)
> > > >>> Dereferencing null pointer "per_buffer_data".
> > > 1295 blk_info = *((uint8 *) per_buffer_data);
> > > 1296 CheckBufferIsPinnedOnce(buf);
> > > 1297 page = BufferGetPage(buf);
> > > 1298 blkno = BufferGetBlockNumber(buf);
> > > 1299
> > > 1300 vacrel->scanned_pages++;
> > >
> > > Basically, Coverity doesn't understand that a successful call to
> > > read_stream_next_buffer must set per_buffer_data here. I don't
> > > think there's much chance of teaching it that, so we'll just
> > > have to dismiss this item as "intentional, not a bug".
> >
> > Is this easy to do? Like is there a list of things from coverity to ignore?
> >
> > > I do have a suggestion: I think the "per_buffer_data" variable
> > > should be declared inside the "while (true)" loop not outside.
> > > That way there is no chance of a value being carried across
> > > iterations, so that if for some reason read_stream_next_buffer
> > > failed to do what we expect and did not set per_buffer_data,
> > > we'd be certain to get a null-pointer core dump rather than
> > > accessing data from a previous buffer.
> >
> > Done and pushed. Thanks!
> >
> Per Coverity.
>
> CID 1592454: (#1 of 1): Explicit null dereferenced (FORWARD_NULL)
> 8. var_deref_op: Dereferencing null pointer per_buffer_data.

That's exactly what the messages you quoted are discussing, no?

> Sorry if I'm wrong, but the function is very suspicious.

How so?

> diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
> index 04bdb5e6d4..18e9b4f3c4 100644
> --- a/src/backend/storage/aio/read_stream.c
> +++ b/src/backend/storage/aio/read_stream.c
> @@ -666,6 +666,8 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
> READ_BUFFERS_ISSUE_ADVICE : 0)))
> {
> /* Fast return. */
> + if (per_buffer_data)
> + *per_buffer_data = get_per_buffer_data(stream, oldest_buffer_index);
> return buffer;
> }

A few lines above:
Assert(stream->per_buffer_data_size == 0);

The fast path isn't used when per buffer data is used. Adding a check for
per_buffer_data and assigning something to it is nonsensical.

Greetings,

Andres Freund