Closed
Bug 1278796
Opened 5 years ago
Closed 5 years ago
[Static Analysis][Uninitialized pointer field] In function PurpleBlock
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: andi, Assigned: mccr8)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, Whiteboard: CID 1362546)
Attachments
(5 files, 1 obsolete file)
1.35 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
2.20 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
1.63 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
1.39 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
1.87 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
The Static Analysis tool Coverity detected that pointer field |mEntries| is not initialized in the constructor. As we plan push for a RAII idiom and to integrate: 525063 we need this kind of issues fixed.
We can memset this pointer in the constructor.
>> // Ensure PurpleBlock is the right size (see above).
>> static_assert(
>> sizeof(PurpleBlock) == 16384 || // 32-bit
>> sizeof(PurpleBlock) == 32768, // 64-bit
>> "ill-sized nsPurpleBuffer::PurpleBlock"
>> );
>> memset(mEntries, 0, sizeof(mEntries));
Reporter | ||
Comment 1•5 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/58462/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/58462/
Reporter | ||
Updated•5 years ago
|
Attachment #8761130 -
Flags: review?(nfroyd)
![]() |
||
Comment 2•5 years ago
|
||
Comment on attachment 8761130 [details] Bug 1278796 - memset member variable mEntries. Andrew is more qualified to review this than I am. My gut feeling here is that we don't want the extra overhead of the memset, since we're going to overwrite the entries in the purple buffer anyway in StartBlock() -- assuming I'm reading this code correctly. So this would be a case where would could instruct a static analysis to ignore the non-initialization of the entries.
Attachment #8761130 -
Flags: review?(nfroyd) → review?(continuation)
Assignee | ||
Comment 3•5 years ago
|
||
Comment on attachment 8761130 [details] Bug 1278796 - memset member variable mEntries. We do initialize it every time we create it, just not in the constructor. I can write a patch to rearrange things a little and hopefully that will make Coverity happy.
Attachment #8761130 -
Flags: review?(continuation) → review-
Assignee | ||
Updated•5 years ago
|
Assignee: bpostelnicu → continuation
Reporter | ||
Comment 4•5 years ago
|
||
Hello Andrew, Maybe it will help Coverity but it won't help us to integrate the clang-plugin, basically what it does it matches the constructor from Ast record declaration and crawls to see if all of the variables are initialized. We can ignore this using flag INITIALIZED_OUTSIDE_CONSTRUCTOR in the declaration of mEntries. Let me know if it's ok with you.
Flags: needinfo?(continuation)
Assignee | ||
Comment 5•5 years ago
|
||
I was going to change the code to initialize the array inside the constructor. I guess right now we don't initialize all of the fields of the elements of the array, but I could add that. Once you are iterating over the array presumably it is mostly free to write some additional data.
Flags: needinfo?(continuation)
Assignee | ||
Comment 6•5 years ago
|
||
How detailed is the checking? If I set mEntries[i].mNextInFreeList for all elements of mEntries, in the ctor, is that sufficient?
Flags: needinfo?(bpostelnicu)
Assignee | ||
Comment 8•5 years ago
|
||
There are a lot of patches here, but each one is a small refactoring. Feel free to ignore this for a while, as I'll be in London next week, and on PTO for most of the week after that, so I might not land it for a few weeks. The end goal of these patches is to explicitly initialize mEntries in the PurpleBlock ctor, rather than in some random function called later. try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d44ce9451c72 It is used inconsistently, which is confusing. A later patch will move these into a PurpleBlock method, so the length shouldn't be an issue.
Attachment #8761665 -
Flags: review?(bugs)
Assignee | ||
Comment 9•5 years ago
|
||
Attachment #8761666 -
Flags: review?(bugs)
Assignee | ||
Comment 10•5 years ago
|
||
This method doesn't do much, and in a later patch it will do even less.
Attachment #8761667 -
Flags: review?(bugs)
Assignee | ||
Comment 11•5 years ago
|
||
This clears the way for the next patch.
Attachment #8761668 -
Flags: review?(bugs)
Assignee | ||
Comment 12•5 years ago
|
||
This makes sure that it will always happen, and should placate static analyses.
Attachment #8761669 -
Flags: review?(bugs)
Assignee | ||
Updated•5 years ago
|
Attachment #8761130 -
Attachment is obsolete: true
Assignee | ||
Comment 13•5 years ago
|
||
(If your review queue is bad, I could ask froydnj to review. These patches do not require any specialized knowledge.)
Comment 14•5 years ago
|
||
Comment on attachment 8761665 [details] [diff] [review] part 1 - Inline |entries| in StartBlock. so you're uninlining entries, I'd say. If compiler doesn't optimize anything out, this makes the code slower, no? Instead of entries[i - 1] we do aBlock->mEntries[i - 1], so one indirect access more. I assume latter patches explain why this.
Attachment #8761665 -
Flags: review?(bugs) → review+
Updated•5 years ago
|
Attachment #8761666 -
Flags: review?(bugs) → review+
Comment 15•5 years ago
|
||
Comment on attachment 8761667 [details] [diff] [review] part 3 - Inline StartBlock. I assume latter patches somehow make this better, since StartBlock looks like a nice way to guarantee mFreeList is set/initialized correctly.
Attachment #8761667 -
Flags: review?(bugs) → review+
Comment 16•5 years ago
|
||
Comment on attachment 8761668 [details] [diff] [review] part 4 - Move InitNextPointers out of InitBlocks. a tad error prone to require more explicit initializations. Again I assume the latter patches make this somehow better.
Attachment #8761668 -
Flags: review?(bugs) → review+
Comment 17•5 years ago
|
||
Comment on attachment 8761669 [details] [diff] [review] part 5 - Move InitNextPointers into the PurpleBlock ctor. ok, I see. A tad annoying that nsPurpleBuffer::SelectPointers needs still two inits (InitBlocks and InitNextPointers) but fine.
Attachment #8761669 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 18•5 years ago
|
||
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #17) > A tad annoying that nsPurpleBuffer::SelectPointers needs still > two inits (InitBlocks and InitNextPointers) but fine. Yeah, it isn't ideal. Fortunately this code doesn't change much. Thanks for the reviews.
Comment 19•5 years ago
|
||
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba19b6e0ea54 part 1 - Inline |entries| in StartBlock. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/420531c2f7ed part 2 - Initialize PurpleBlock next pointers in a method. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/7e5c2c10d43b part 3 - Inline StartBlock. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/2589d6ae710c part 4 - Move InitNextPointers out of InitBlocks. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/99582cfde7c2 part 5 - Move InitNextPointers into the PurpleBlock ctor. r=smaug
Comment 20•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ba19b6e0ea54 https://hg.mozilla.org/mozilla-central/rev/420531c2f7ed https://hg.mozilla.org/mozilla-central/rev/7e5c2c10d43b https://hg.mozilla.org/mozilla-central/rev/2589d6ae710c https://hg.mozilla.org/mozilla-central/rev/99582cfde7c2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•