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

RESOLVED FIXED in Firefox 46

Status

()

Core
XPCOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
mozilla46
coverity
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: CID 1324687)

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8701037 [details] [diff] [review]
Bug 1234535.diff
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+
(Assignee)

Comment 3

2 years ago
Created attachment 8703590 [details] [diff] [review]
Bug 1234535.diff

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+
(Assignee)

Comment 5

2 years ago
Created attachment 8703610 [details] [diff] [review]
Bug 1234535.diff
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+
(Assignee)

Comment 7

2 years ago
Created attachment 8703616 [details] [diff] [review]
Bug 1234535.diff
Attachment #8703610 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 9

2 years ago
Created attachment 8703628 [details] [diff] [review]
Bug 1234535.diff

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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Thank you!

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/49ec722b985f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.