From a9284849b48b04fa2836aaf704659974c13e610d Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 3 Apr 2016 14:17:20 -0400 Subject: [PATCH] Clean up some stuff in new contrib/bloom module. Coverity complained about implicit sign-extension in the BloomPageGetFreeSpace macro, probably because sizeOfBloomTuple isn't wide enough for size calculations. No overflow is really possible as long as maxoff and sizeOfBloomTuple are small enough to represent a realistic situation, but it seems like a good idea to declare sizeOfBloomTuple as Size not int32. Add missing check on BloomPageAddItem() result, again from Coverity. Avoid core dump due to not allocating so->sign array when scan->numberOfKeys is zero. Also thanks to Coverity. Use FLEXIBLE_ARRAY_MEMBER rather than declaring an array as size 1 when it isn't necessarily. Very minor beautification of related code. Unfortunately, none of the Coverity-detected mistakes look like they could account for the remaining buildfarm unhappiness with this module. It's barely possible that the FLEXIBLE_ARRAY_MEMBER mistake does account for that, if it's enabling bogus compiler optimizations; but I'm not terribly optimistic. We probably still have bugs to find here. --- contrib/bloom/blinsert.c | 11 ++++++++--- contrib/bloom/bloom.h | 4 ++-- contrib/bloom/blscan.c | 7 +++++-- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c index 9e6678087c..6eecb12187 100644 --- a/contrib/bloom/blinsert.c +++ b/contrib/bloom/blinsert.c @@ -96,10 +96,10 @@ bloomBuildCallback(Relation index, HeapTuple htup, Datum *values, initCachedPage(buildstate); - if (BloomPageAddItem(&buildstate->blstate, buildstate->data, itup) == false) + if (!BloomPageAddItem(&buildstate->blstate, buildstate->data, itup)) { /* We shouldn't be here since we're inserting to the empty page */ - elog(ERROR, "can not add new tuple"); + elog(ERROR, "could not add new bloom tuple to empty page"); } } @@ -298,7 +298,12 @@ blinsert(Relation index, Datum *values, bool *isnull, metaData = BloomPageGetMeta(metaPage); page = GenericXLogRegister(state, buffer, true); BloomInitPage(page, 0); - BloomPageAddItem(&blstate, page, itup); + + if (!BloomPageAddItem(&blstate, page, itup)) + { + /* We shouldn't be here since we're inserting to the empty page */ + elog(ERROR, "could not add new bloom tuple to empty page"); + } metaData->nStart = 0; metaData->nEnd = 1; diff --git a/contrib/bloom/bloom.h b/contrib/bloom/bloom.h index fb0bc07f28..8f3881d844 100644 --- a/contrib/bloom/bloom.h +++ b/contrib/bloom/bloom.h @@ -118,7 +118,7 @@ typedef struct BloomState * sizeOfBloomTuple is index's specific, and it depends on reloptions, so * precompute it */ - int32 sizeOfBloomTuple; + Size sizeOfBloomTuple; } BloomState; #define BloomPageGetFreeSpace(state, page) \ @@ -134,7 +134,7 @@ typedef uint16 SignType; typedef struct BloomTuple { ItemPointerData heapPtr; - SignType sign[1]; + SignType sign[FLEXIBLE_ARRAY_MEMBER]; } BloomTuple; #define BLOOMTUPLEHDRSZ offsetof(BloomTuple, sign) diff --git a/contrib/bloom/blscan.c b/contrib/bloom/blscan.c index d156e88669..6e3cb84bb1 100644 --- a/contrib/bloom/blscan.c +++ b/contrib/bloom/blscan.c @@ -94,7 +94,7 @@ blgetbitmap(IndexScanDesc scan, TIDBitmap *tbm) BufferAccessStrategy bas; BloomScanOpaque so = (BloomScanOpaque) scan->opaque; - if (so->sign == NULL && scan->numberOfKeys > 0) + if (so->sign == NULL) { /* New search: have to calculate search signature */ ScanKey skey = scan->keyData; @@ -151,10 +151,13 @@ blgetbitmap(IndexScanDesc scan, TIDBitmap *tbm) bool res = true; /* Check index signature with scan signature */ - for (i = 0; res && i < so->state.opts->bloomLength; i++) + for (i = 0; i < so->state.opts->bloomLength; i++) { if ((itup->sign[i] & so->sign[i]) != so->sign[i]) + { res = false; + break; + } } /* Add matching tuples to bitmap */ -- 2.40.0