Em sáb., 18 de jul. de 2020 às 14:21, Tom Lane <t...@sss.pgh.pa.us> escreveu:
> Ranier Vilela <ranier...@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
