Closed Bug 1232665 Opened 4 years ago Closed 4 years ago

[Static Analysis][Uninitialized scalar field] In function gfxFontGroup::gfxFontGroup(const FontFamilyList& aFontFamilyList,..) from gfxTextRun.cpp

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1325709 )

Attachments

(1 file)

The Static Analysis tool Coverity added that member variables:  mLastPrefLang and  mLastPrefFirstFont are not initialized in the constructor. Although the two class memebrs are not used without initialization due to the fact that first time mLastPrefFont returns null pointer in:

>>   // if the last pref font was the first family in the pref list, no need to recheck through a list of families
>>   if (mLastPrefFont && charLang == mLastPrefLang &&
>>       mLastPrefFirstFont && mLastPrefFont->HasCharacter(aCh)) {
>>       font = mLastPrefFont;
>>       return font.forget();
>>   }   

toughs the rest of the condition is not evaluated and execution continues on the false branch so garbage values of the two memebr variables are not taken into account i would suggest to initialize them for two reasons: 
1. prevent side effect of future use of this class when this kind of mechanics are not followed.
2. silence coverity.
Attached patch Bug 1232665.diffSplinter Review
Attachment #8698437 - Flags: review?(jmuizelaar)
Comment on attachment 8698437 [details] [diff] [review]
Bug 1232665.diff

Review of attachment 8698437 [details] [diff] [review]:
-----------------------------------------------------------------

Jonathan Kew is a better person to look at this
Attachment #8698437 - Flags: review?(jmuizelaar) → review?(jfkthame)
In general, I don't much like adding extra code just to silence a tool, when the code is actually correct as-is and the issue is that the tool isn't smart enough to understand it.

Does coverity support some kind of annotation that we can put in a comment to help it here?
Comment on attachment 8698437 [details] [diff] [review]
Bug 1232665.diff

This seems fine to me.
Attachment #8698437 - Flags: feedback+
For uninitialized member variables i don't think that Coveirty scans the code to actually see if these are used and in what conditions, it just triggers the warning when it runs to the constructor.

Coverity has support for function ad notation but i think it's limited. It omly has support for three tags: kill, alloc and free.

You can teach him that a function frees memory, for example the following specifies that my_free() always frees memory assigned to
its first position argument (memory_to_free) without a dereference::

>>// coverity[+free : arg-1]
>>void my_free(void** arg, void* memory_to_free)
>>{ ... }

So i don't think we can ad notation in our particular case. If you think that we should not have this patch maybe we can mark the issue as invalid?
Comment on attachment 8698437 [details] [diff] [review]
Bug 1232665.diff

Review of attachment 8698437 [details] [diff] [review]:
-----------------------------------------------------------------

I guess it's fine to do this to keep coverity happy, although strictly speaking it's redundant.
Attachment #8698437 - Flags: review?(jfkthame) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3f06ed0dd635
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.