Thread: CID 1428952 (#1 of 1): Out-of-bounds access (OVERRUN) (src/backend/commands/async.c)

Ranier Vilela <ranier.vf@gmail.com> writes:
> Can you take a look?
> Per Coverity.
> There is something wrong with the definition of QUEUE_PAGESIZE on async.c

No, there's just something wrong with Coverity's analysis.
I've grown a bit disillusioned with that tool; of late it's
been giving many more false positives than useful reports.

> 3..sizeof(AsyncQueueControl) is 8080, according to Coverity (Windows 64
> bits)

ITYM AsyncQueueEntry?

> 4. (Line 1508)    qe.length = QUEUE_PAGESIZE - offset;
> 5. offset is zero
> 6. qe.length is 8192
> /* Now copy qe into the shared buffer page */
> memcpy(NotifyCtl->shared->page_buffer[slotno] + offset,
>   &qe,
>   qe.length);
> CID 1428952 (#1 of 1): Out-of-bounds access (OVERRUN)  at line 1515, with
> memcpy call.
> 9. overrun-buffer-arg: Overrunning struct type AsyncQueueEntry of 8080
> bytes by passing it to a function which accesses it at byte offset 8191
> using argument qe.length (which evaluates to 8192).

I suppose what Coverity is on about is the possibility that we might
increase qe.length to more than sizeof(AsyncQueueEntry).  However,
given the logic:

        if (offset + qe.length <= QUEUE_PAGESIZE)
            ...
        else
            qe.length = QUEUE_PAGESIZE - offset;

that assignment must be *reducing* qe.length, so there can be no overrun
unless asyncQueueNotificationToEntry() had prepared an oversize value to
begin with.  Which is impossible given the assertions in that function,
but maybe Coverity can't work that out?  (But then why isn't it
complaining about asyncQueueNotificationToEntry itself?)

I'd be willing to add a relevant assertion to
asyncQueueNotificationToEntry, along the lines of

    /* The terminators are already included in AsyncQueueEntryEmptySize */
    entryLength = AsyncQueueEntryEmptySize + payloadlen + channellen;
    entryLength = QUEUEALIGN(entryLength);
+    Assert(entryLength <= sizeof(AsyncQueueEntry));
    qe->length = entryLength;
    qe->dboid = MyDatabaseId;
    qe->xid = GetCurrentTransactionId();

if it'd shut up Coverity on this point; but I have no easy way
to find that out.

> Question:
> 1. NotifyCtl->shared->page_buffer[slotno] is really struct type
> AsyncQueueEntry?

No, it's a page.  But it contains AsyncQueueEntry(s).

            regards, tom lane

Ranier Vilela <ranier.vf@gmail.com> writes:
> Em sáb., 18 de jul. de 2020 às 14:21, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
>> No, there's just something wrong with Coverity's analysis.
>> I've grown a bit disillusioned with that tool; of late it's
>> been giving many more false positives than useful reports.

> It could be, differences in the sizes of the types. Since on Linux, there
> may be no alerts.

No, all the types involved here should be pretty platform-independent.
IIRC, the PG security team already saw this same warning from Coverity,
and we dismissed it as a false positive.

            regards, tom lane