1338544 - [Static Analysis][Dereference after null check] In function Predictor::UpdateCacheability

Status: RESOLVED WONTFIX

(Core :: Networking, defect, P3)

Description

[Andi [:andi]]

The Static Analysis tool Coverity detected that variable |lci| is dereferenced even if it's null:

dereference:

>>    self->UpdateCacheabilityInternal(sourceURI, targetURI, httpStatus,
>>                                     method, *lci->OriginAttributesPtr());

Coverity indicates that |lci| could be null because of the following null comparison:

>>  if (lci && lci->IsPrivate()) {
>>    PREDICTOR_LOG(("Predictor::UpdateCacheability in PB mode - ignoring"));
>>    return;
>>  }

This assumption is wrong since be looking at the call stack:
>>    if (referrer) {
>>        nsCOMPtr<nsILoadContextInfo> lci = GetLoadContextInfo(this);
>>        mozilla::net::Predictor::UpdateCacheability(referrer, mURI, httpStatus,
>>                                                    mRequestHead, mResponseHead,
>>                                                    lci);
>>    }

So what i want to do to silent the checker, is to remove the null check.

Comment 1

[Andi [:andi]]

Attachment: Bug 1338544 - fix false positive for static analysis in Predictor::UpdateCacheability.

Review commit: https://reviewboard.mozilla.org/r/111548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111548/

Comment 2

[u408661]

Comment on attachment 8836064 [details]
Bug 1338544 - fix false positive for static analysis in Predictor::UpdateCacheability.

https://reviewboard.mozilla.org/r/111548/#review112878

Coverity (as you point out) is horribly wrong here. However, removing the null check is not the right way to go about fixing that (which... how does that even fix coverity complaining about a potentially null pointer?!) Long story short, coverity obviously doesn't understand c++ short-circuit rules (the form |lci && lci->IsPrivate()| is a perfectly legitimate way of ensuring |lci| isn't null-derefed in c++). So, either we need to whitelist this in coverity (preferred, in my opinion, if possible), or just change the check so that coverity can figure it out (nested ifs, in this case).

Comment 3

[Andi [:andi]]

by removing the null check we instruct Coverity that we don't expect pointer |lci| to be null thus when dereferenciating here:

>>    self->UpdateCacheabilityInternal(sourceURI, targetURI, httpStatus,
>>                                     method, *lci->OriginAttributesPtr());

will not trigger a coveirty checker, since this checker is triggered due to the previous null pointer check before the deference:

>>  if (lci && lci->IsPrivate()) {
>>    PREDICTOR_LOG(("Predictor::UpdateCacheability in PB mode - ignoring"));
>>    return;
>>  }

Comment 4

[u408661]

OK, so instead of removing a null check, check that before using lci in the call above. You'll probably have to use a ternary operator in the function call itself for it to make sense. Removing a null check should never be the thing you do.

Comment 5

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 7

[Andi [:andi]]

Closing this, as this is a false positive and moving on with coverity updates this issue has been removed from the defects list. Coverity became smarter.