Closed
Bug 1232665
Opened 8 years ago
Closed 8 years ago
[Static Analysis][Uninitialized scalar field] In function gfxFontGroup::gfxFontGroup(const FontFamilyList& aFontFamilyList,..) from gfxTextRun.cpp
Categories
(Core :: Graphics, defect)
Core
Graphics
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)
1.04 KB,
patch
|
jfkthame
:
review+
jtd
:
feedback+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8698437 -
Flags: review?(jmuizelaar)
Comment 2•8 years ago
|
||
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)
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
Comment on attachment 8698437 [details] [diff] [review] Bug 1232665.diff This seems fine to me.
Attachment #8698437 -
Flags: feedback+
Assignee | ||
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3f06ed0dd635
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
•