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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andi-Bogdan Postelnicu [:andi]]

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));

Comment 1

[Andi-Bogdan Postelnicu [:andi]]

Attachment: Bug 1278796 - memset member variable mEntries.

Review commit: https://reviewboard.mozilla.org/r/58462/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58462/

Comment 2

[Nathan Froyd [:froydnj] [away until 28 October]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Andi-Bogdan Postelnicu [:andi]]

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.

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Andrew McCreight [:mccr8]]

How detailed is the checking?  If I set mEntries[i].mNextInFreeList for all elements of mEntries, in the ctor, is that sufficient?

Comment 7

[Andi-Bogdan Postelnicu [:andi]]

yes, i think that should do it.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Inline |entries| in StartBlock.

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.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Initialize PurpleBlock next pointers in a method.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Inline StartBlock.

This method doesn't do much, and in a later patch it will do even less.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Move InitNextPointers out of InitBlocks.

This clears the way for the next patch.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: part 5 - Move InitNextPointers into the PurpleBlock ctor.

This makes sure that it will always happen, and should placate static analyses.

Comment 13

[Andrew McCreight [:mccr8]]

(If your review queue is bad, I could ask froydnj to review. These patches do not require any specialized knowledge.)

Comment 14

[Olli Pettay [:smaug]]

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.

Comment 15

[Olli Pettay [:smaug]]

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.

Comment 16

[Olli Pettay [:smaug]]

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.

Comment 17

[Olli Pettay [:smaug]]

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.

Comment 18

[Andrew McCreight [:mccr8]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

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