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

Em sáb., 18 de jul. de 2020 às 14:21, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> escreveu:

> Ranier Vilela <ranier(dot)vf(at)gmail(dot)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