Re: Confine vacuum skip logic to lazy_scan_skip

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.

I think that function *read_stream_next_buffer* can return
a invalid per_buffer_data pointer, with a valid buffer.

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

Attached a patch, which tries to fix.

best regards,
Ranier Vilela