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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andi [:andi]]

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.

Comment 1

[Andi [:andi]]

Attachment: Bug 1234535.diff

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[Andi [:andi]]

Attachment: 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.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8703590 [details] [diff] [review]
Bug 1234535.diff

Looks good, but please update the commit message. :)

Comment 5

[Andi [:andi]]

Attachment: Bug 1234535.diff

Comment 6

[Nathan Froyd [:froydnj]]

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.

Comment 7

[Andi [:andi]]

Attachment: Bug 1234535.diff

Comment 8

[Nathan Froyd [:froydnj]]

Sorry, noticed that should be "avoid spurious null dereference complaint from Coverity"; could you change that, please?

Comment 9

[Andi [:andi]]

Attachment: Bug 1234535.diff

sure here it goes.

Comment 10

[Nathan Froyd [:froydnj]]

Thank you!

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49ec722b985f

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49ec722b985f