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

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks 1 bug, {coverity})

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: CID 1354259 [necko-active])

Attachments

(1 attachment)

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);
>>    }
Assignee

Comment 1

3 years ago
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-
Assignee

Comment 4

3 years ago
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.
Assignee

Updated

3 years ago
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)
Assignee

Comment 5

3 years ago
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+
Assignee

Comment 8

3 years ago
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/

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b73632b2d724
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.