Re: Confine vacuum skip logic to lazy_scan_skip

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!

- Melanie