Closed Bug 1338544 Opened 3 years ago Closed 6 months ago

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

Categories

(Core :: Networking, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox54 --- affected

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1400173 [necko-next])

Attachments

(1 file)

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.
Attachment #8836064 - Flags: review?(valentin.gosu) → review?(hurley)
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).
Attachment #8836064 - Flags: review?(hurley) → review-
Whiteboard: CID 1400173 → CID 1400173 [necko-next]
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;
>>  }
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.
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P2
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
Priority: P2 → P3

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.

Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.