Re: Confine vacuum skip logic to lazy_scan_skip

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!