Closed Bug 1251253 Opened 5 years ago Closed 5 years ago

[Static Analysis][Dereference after null check] In function CacheStorageService::DoomStorageEntries

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1354259 [necko-active])

Attachments

(1 file)

The Static Analysis tool Coverity added that variable |aContext| can cause a null pointer dereference in the following context:

>>  {
>>    mozilla::MutexAutoLock lock(mForcedValidEntriesLock);
>>
>>    for (auto iter = mForcedValidEntries.Iter(); !iter.Done(); iter.Next()) {
>>      bool matches;
>>      DebugOnly<nsresult> rv = CacheFileUtils::KeyMatchesLoadContextInfo(
>>        iter.Key(), aContext, &matches);
>>      MOZ_ASSERT(NS_SUCCEEDED(rv));
>>
>>      if (matches) {
>>        iter.Remove();
>>      }
>>    }
>>  }

This checker was triggered by coverity because of an earlier null check on |aContext| :

>>    if (aContext && !aContext->IsPrivate()) {
>>      LOG(("  dooming disk entries"));
>>      CacheFileIOManager::EvictByContext(aContext, aPinned);
>>    }
I think this is more prone to happen since DoomStorageEntries can get called with |aContext| being explicitly nullptr:

>>  nsTArray<nsCString> keys;
>>  for (auto iter = sGlobalEntryTables->Iter(); !iter.Done(); iter.Next()) {
>>    const nsACString& key = iter.Key();
>>    nsCOMPtr<nsILoadContextInfo> info = CacheFileUtils::ParseKey(key);
>>    if (info && info->IsPrivate()) {
>>      keys.AppendElement(key);
>>    }
>>  }
>>
>>  for (uint32_t i = 0; i < keys.Length(); ++i) {
>>    DoomStorageEntries(keys[i], nullptr, true, false, nullptr);
>>  }
Attachment #8723578 - Flags: review?(mcmanus) → review?(honzab.moz)
Comment on attachment 8723578 [details]
MozReview Request: Bug 1251253 - prevent null pointer dereference of |aContext| in CacheStorageService::DoomStorageEntries. r?mayhemer

Good catch!

But the patch is not fully correct.  aContext may be null.  In that case we want to delete everything.  

So, when aContext is null just delete the whole mForcedValidEntries table.

BTW, I don't see any dereference here.  We pass it to KeyMatchesLoadContextInfo which is fully capable of null-checking...
Attachment #8723578 - Flags: review?(honzab.moz) → review-
Thanks for letting me know i will update the patch. 
Now regarding the null dereference:

in function CacheFileUtils::KeyMatchesLoadContextInfo that takes as 2nd argument |aContext| is transferred as |aInfo| and we have:

>>nsresult
>>KeyMatchesLoadContextInfo(const nsACString &aKey, nsILoadContextInfo *aInfo,
>>                          bool *_retval)
>>{
>>  nsCOMPtr<nsILoadContextInfo> info = ParseKey(aKey);
>>
>>  if (!info) {
>>    return NS_ERROR_FAILURE;
>>  }
>>
>>  *_retval = info->Equals(aInfo);
>>  return NS_OK;
>>}

next |aInfo| is transferred as |aOther|:

>>  bool Equals(nsILoadContextInfo *aOther)
>>  {
>>    return IsPrivate() == aOther->IsPrivate() &&
>>           IsAnonymous() == aOther->IsAnonymous() &&
>>           *OriginAttributesPtr() == *aOther->OriginAttributesPtr();
>>  }

moving on in IsPrivate: 

>>  bool IsPrivate()
>>  {
>>    bool pb;
>>    GetIsPrivate(&pb);
>>    return pb;
>>  }

dereference happens on line 3:
>>    GetIsPrivate(&pb);

because GetIsPrivate is virtual so this will be dereferenced in order to resolve the function from vtable.
Attachment #8723578 - Attachment description: MozReview Request: Bug 1251253 - prevent null pointer dereference of |aContext| in CacheStorageService::DoomStorageEntries. r?mcmanus → MozReview Request: Bug 1251253 - prevent null pointer dereference of |aContext| in CacheStorageService::DoomStorageEntries. r?mayhemer
Attachment #8723578 - Flags: review- → review?(honzab.moz)
Comment on attachment 8723578 [details]
MozReview Request: Bug 1251253 - prevent null pointer dereference of |aContext| in CacheStorageService::DoomStorageEntries. r?mayhemer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36631/diff/1-2/
Whiteboard: CID 1354259 → CID 1354259 [necko-active]
(In reply to Andi-Bogdan Postelnicu from comment #4)
> Thanks for letting me know i will update the patch. 
> Now regarding the null dereference:

Yes!  You are right.  I misread the argument name.  Thanks.
Comment on attachment 8723578 [details]
MozReview Request: Bug 1251253 - prevent null pointer dereference of |aContext| in CacheStorageService::DoomStorageEntries. r?mayhemer

https://reviewboard.mozilla.org/r/36631/#review33379

::: netwerk/cache2/CacheStorageService.cpp:1801
(Diff revision 2)
> -
> +    if (aContext) {

leave the blank line after mozilla::MutexAutoLock lock(..);

::: netwerk/cache2/CacheStorageService.cpp:1813
(Diff revision 2)
> +    else {

nit:

} else {
Attachment #8723578 - Flags: review?(honzab.moz) → review+
Comment on attachment 8723578 [details]
MozReview Request: Bug 1251253 - prevent null pointer dereference of |aContext| in CacheStorageService::DoomStorageEntries. r?mayhemer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36631/diff/2-3/
https://hg.mozilla.org/mozilla-central/rev/b73632b2d724
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.