Closed
Bug 1234535
Opened 8 years ago
Closed 8 years ago
[Static Analysis][Dereference null return value] In function nsRuleNode::ConvertChildrenToHash from PLDHashTable.cpp
Categories
(Core :: XPCOM, defect)
Core
XPCOM
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)
1.16 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
Attachment #8701037 -
Flags: review?(nfroyd)
Comment 2•8 years ago
|
||
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•8 years ago
|
||
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 4•8 years ago
|
||
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•8 years ago
|
||
Attachment #8703590 -
Attachment is obsolete: true
Attachment #8703610 -
Flags: review?(nfroyd)
Comment 6•8 years ago
|
||
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•8 years ago
|
||
Attachment #8703610 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 8•8 years ago
|
||
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•8 years ago
|
||
sure here it goes.
Attachment #8703616 -
Attachment is obsolete: true
Flags: needinfo?(bogdan.postelnicu)
Attachment #8703628 -
Flags: review?(nfroyd)
Updated•8 years ago
|
Attachment #8703628 -
Flags: review?(nfroyd) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
Thank you!
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/49ec722b985f
Keywords: checkin-needed
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/49ec722b985f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•