Closed Bug 1278796 Opened 3 years ago Closed 3 years ago

[Static Analysis][Uninitialized pointer field] In function PurpleBlock

Categories

(Core :: XPCOM, defect)

defect
Not set

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)

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));
Attachment #8761130 - Flags: review?(nfroyd)
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)
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: bpostelnicu → continuation
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)
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)
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)
yes, i think that should do it.
Flags: needinfo?(bpostelnicu)
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)
This method doesn't do much, and in a later patch it will do even less.
Attachment #8761667 - Flags: review?(bugs)
This clears the way for the next patch.
Attachment #8761668 - Flags: review?(bugs)
This makes sure that it will always happen, and should placate static analyses.
Attachment #8761669 - Flags: review?(bugs)
Attachment #8761130 - Attachment is obsolete: true
(If your review queue is bad, I could ask froydnj to review. These patches do not require any specialized knowledge.)
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+
Attachment #8761666 - Flags: review?(bugs) → review+
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 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 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+
(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.
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
You need to log in before you can comment on or make changes to this bug.