1293956 - Spike in number of coverity defect caused by "Misused comma operator"

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

A recent change caused in the number of Coverity defects.

This created a huge spike for Firefox:
New defects found: 4045
And Thunderbird:
New defects found: 545

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Andi, could you have a look when you go back from pto? Thanks

Comment 2

[Sylvestre Ledru [:Sylvestre]]

To be clear, this is not caused by a new version of coverity
The run of August first was done with 8.5.0.1:
http://relman-ci.mozilla.org/job/firefox-coverity/150/consoleFull
We would have get it at the time. I think it is a change in the code (in a header, very likely).

Comment 3

[Andi [:andi]]

After discussing with Mark Armitage, a contact from Coverity, we realised that this spike appeared due to the update from 7.7.0.4 to the 8.5.0. Bellow is a quote from his email:

>>I see that your project gained 4072 new defects on the 9th of this month, 
>>and this was the first analysis done with version 8.5.0 - the previous
>>analysis was with version 7.7.0.4.  So it’s almost certainly something
>>related to this change.  The other interesting change is that in that same
>>run we over 100,000 more LOC in your project than in the previous run.
>>So either something changed in your build, or (more likely) we may have
>>fixed some bugs that were preventing some of your code being analyzed
>>previously - so the 4000 extra defects are because we are analyzing more code!

I will continue to investigate this with his support.
For example Coverity ID 1368264:

   	
>>CID 1368264 (#1 of 1): Misused comma operator (NO_EFFECT)
>>extra_comma: Part this->mRefCnt of statement (this->mRefCnt) , false has no effect due to the comma.
>> 12NS_IMPL_ADDREF(nsSystemAlertsService)

>>The details about this error are:
>>The left hand side of the comma will be evaluated and then the value discarded.
>>In nsSystemAlertsService::​AddRef(): Comma operator has a left sub-expression with no side-effects (CWE-480)


The current problem seems to be related with the macro definition for NS_IMPL_ADDREF

>>#define NS_IMPL_ADDREF(_class)                                                \
>>NS_IMETHODIMP_(MozExternalRefCountType) _class::AddRef(void)                  \
>>{                                                                             \
>>  MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING(_class)                                  \
>>  MOZ_ASSERT(int32_t(mRefCnt) >= 0, "illegal refcnt");                        \
>>  if (!mRefCnt.isThreadSafe)                                                  \
>>    NS_ASSERT_OWNINGTHREAD(_class);                                           \
>>  nsrefcnt count = ++mRefCnt;                                                 \
>>  NS_LOG_ADDREF(this, count, #_class, sizeof(*this));                         \
>>  return count;                                                               \
>>}

>>#define NS_IMETHODIMP_(type) type

I don’t see an error here, the only think that i can think of that Coverity is suspicious about the cast int32_t(mRefCnt),
where the type of mRefCnt is nsAutoRefCnt and gets casted to int32_t by using the overloading of operator nsrefcnt:

>>class nsAutoRefCnt
>>{
>>public:
>>  nsAutoRefCnt() : mValue(0) {}
>>  explicit nsAutoRefCnt(nsrefcnt aValue) : mValue(aValue) {}
>>
>>  // only support prefix increment/decrement
>>  nsrefcnt operator++() { return ++mValue; }
>>  nsrefcnt operator--() { return --mValue; }
>>
>>  nsrefcnt operator=(nsrefcnt aValue) { return (mValue = aValue); }
>>  operator nsrefcnt() const { return mValue; }
>>  nsrefcnt get() const { return mValue; }
>>
>>  static const bool isThreadSafe = false;
>>private:
>>  nsrefcnt operator++(int) = delete;
>>  nsrefcnt operator--(int) = delete;
>>  nsrefcnt mValue;
>>};

I'll update this bug when there are new findings.

Comment 4

[Andi [:andi]]

After investigating this issue with Mark, our poc from Coverity the conclusion was that this is a bug in their system that will be addressed when they release 8.5 scanner.

Comment 5

[Andi [:andi]]

This issue can now be closed since by upgrading to the latest coverity scan, around 3100 false positive issues were removed.