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

Hi Tom,

Can you take a look?

Per Coverity.

There is something wrong with the definition of QUEUE_PAGESIZE on async.c

1. #define QUEUE_PAGESIZE BLCKSZ
2. BLCKSZ is  8192
3..sizeof(AsyncQueueControl) is 8080, according to Coverity (Windows 64 bits)
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).

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

regards,
Ranier Vilela
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



Em sáb., 18 de jul. de 2020 às 14:21, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
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.
For other projects, it has helped me, but for Postgres it has really been a challenge.

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

ITYM AsyncQueueEntry?
Yes, my bad. Its AsyncQueueEntry size on Windows 64 bits.

> 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;
Here, the offset is zero. Maybe qe.length > QUEUE_PAGESIZE?
"7. Condition offset + qe.length <= 8192, taking false branch."
 

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? 
Coverity analysed the DEBUG version, what includes assertions.
 
(But then why isn't it
complaining about asyncQueueNotificationToEntry itself?)
 I still couldn't say.

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.
I'm not sure that assertion interferes with the analysis.
 

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

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

It could be, differences in the sizes of the types. Since on Linux, there may be no alerts.
But as it was compiled on Windows, the AsyncQueueEntry structure, have a smaller size than in Linux, being smaller than BLCKSZ?
 
regards,
Ranier Vilela
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



Em sáb., 18 de jul. de 2020 às 15:19, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
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.
Understood, again, thanks for your time.

regards,
Ranier Vilela