Closed Bug 1234535 Opened 4 years ago Closed 4 years ago

[Static Analysis][Dereference null return value] In function nsRuleNode::ConvertChildrenToHash from PLDHashTable.cpp

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1324687)

Attachments

(1 file, 4 obsolete files)

The Static Analysis tool Coverity added that in case entry is null, a null pointer derefence will happen:

>>    auto entry =
>>      static_cast<ChildrenHashEntry*>(hash->Add(curr->mRule, fallible));
>>    NS_ASSERTION(!entry->mRuleNode, "duplicate entries in list");
>>    entry->mRuleNode = curr;

entry can be null if in function: PLDHashTable::Add

>>  if (!mEntryStore.Get()) {
>>    uint32_t nbytes;
>>    // We already checked this in the constructor, so it must still be true.
>>    MOZ_RELEASE_ASSERT(SizeOfEntryStore(CapacityFromHashShift(), mEntrySize,
>>                                        &nbytes));
>>    mEntryStore.Set((char*)malloc(nbytes));
>>    if (!mEntryStore.Get()) {
>>      return nullptr;
>>    }
>>    memset(mEntryStore.Get(), 0, nbytes);
>>  }

malloc fails due to oom.
When this happens i think we should have an assert that validates entry.
Attached patch Bug 1234535.diff (obsolete) — Splinter Review
Attachment #8701037 - Flags: review?(nfroyd)
Comment on attachment 8701037 [details] [diff] [review]
Bug 1234535.diff

Review of attachment 8701037 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsRuleNode.cpp
@@ +1649,5 @@
>    PLDHashTable *hash = new PLDHashTable(&ChildrenHashOps,
>                                          sizeof(ChildrenHashEntry),
>                                          aNumKids);
>    for (nsRuleNode* curr = ChildrenList(); curr; curr = curr->mNextSibling) {
>      // This will never fail because of the initial size we gave the table.

Assuming this comment is true...

@@ +1651,5 @@
>                                          aNumKids);
>    for (nsRuleNode* curr = ChildrenList(); curr; curr = curr->mNextSibling) {
>      // This will never fail because of the initial size we gave the table.
>      auto entry =
>        static_cast<ChildrenHashEntry*>(hash->Add(curr->mRule, fallible));

...then this should not be a fallible Add, but an infallible one:

  static_cast<...>(hash->Add(curr->mRule));

@@ +1652,5 @@
>    for (nsRuleNode* curr = ChildrenList(); curr; curr = curr->mNextSibling) {
>      // This will never fail because of the initial size we gave the table.
>      auto entry =
>        static_cast<ChildrenHashEntry*>(hash->Add(curr->mRule, fallible));
> +    NS_ASSERTION(entry, "entry must be a valid pointer");

...and then we won't need this assertion.
Attachment #8701037 - Flags: review?(nfroyd) → feedback+
Attached patch Bug 1234535.diff (obsolete) — Splinter Review
I see your point, i did the add infallible, in this way if we'are OOM the memory management will be aware and also give peace of mind to the static analysis tool.
Attachment #8701037 - Attachment is obsolete: true
Attachment #8703590 - Flags: review?(nfroyd)
Comment on attachment 8703590 [details] [diff] [review]
Bug 1234535.diff

Looks good, but please update the commit message. :)
Attachment #8703590 - Flags: review?(nfroyd) → feedback+
Attached patch Bug 1234535.diff (obsolete) — Splinter Review
Attachment #8703590 - Attachment is obsolete: true
Attachment #8703610 - Flags: review?(nfroyd)
Comment on attachment 8703610 [details] [diff] [review]
Bug 1234535.diff

Review of attachment 8703610 [details] [diff] [review]:
-----------------------------------------------------------------

How about "avoid spurious null dereference complain from Coverity". r=me with that change.
Attachment #8703610 - Flags: review?(nfroyd) → review+
Attached patch Bug 1234535.diff (obsolete) — Splinter Review
Attachment #8703610 - Attachment is obsolete: true
Keywords: checkin-needed
Sorry, noticed that should be "avoid spurious null dereference complaint from Coverity"; could you change that, please?
Flags: needinfo?(bogdan.postelnicu)
Keywords: checkin-needed
Attached patch Bug 1234535.diffSplinter Review
sure here it goes.
Attachment #8703616 - Attachment is obsolete: true
Flags: needinfo?(bogdan.postelnicu)
Attachment #8703628 - Flags: review?(nfroyd)
Attachment #8703628 - Flags: review?(nfroyd) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/49ec722b985f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.