Re: pgsql: Logical Tape Set: lazily allocate read buffer. - Mailing list pgsql-committers

Jeff Davis <jdavis@postgresql.org> writes:
> Logical Tape Set: lazily allocate read buffer.

Coverity is not very pleased with this patch: it's spewing warnings like

1112         if (lt->buffer == NULL)
1113             ltsInitReadBuffer(lts, lt);
1114
1115         if (blocknum != lt->curBlockNumber)
1116         {
>>>     CID 1458440:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "lt->buffer" to "ltsReadBlock", which dereferences it.
1117             ltsReadBlock(lts, blocknum, (void *) lt->buffer);
1118             lt->curBlockNumber = blocknum;

Evidently, it doesn't think that ltsInitReadBuffer() is guaranteed to
make lt->buffer not null, which is not so surprising considering
that that's coded this way:

    if (lt->firstBlockNumber != -1L)
    {
        Assert(lt->buffer_size > 0);
        lt->buffer = palloc(lt->buffer_size);
    }

Is there a reason for that to be coded in such an obscure and fragile
fashion, rather than having the test be say "if (lt->buffer == NULL)"?
If we really need a connection to firstBlockNumber, I'd suggest
something like

-   if (lt->firstBlockNumber != -1L)
+   if (lt->buffer == NULL)
    {
+       Assert(lt->firstBlockNumber != -1L);
        Assert(lt->buffer_size > 0);
        lt->buffer = palloc(lt->buffer_size);
    }

but I'm not sure the extra Assert has any value.

            regards, tom lane