Re: Confine vacuum skip logic to lazy_scan_skip - Mailing list pgsql-hackers

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@gmail.com> escreveu:
> 
> > On Sun, Feb 16, 2025 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >
> > > Thomas Munro <thomas.munro@gmail.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