Closed
Bug 1117596
Opened 9 years ago
Closed 9 years ago
False positive warning "Use of uninitialized |overflowLines|" in nsBlockFrame::AddFrames()
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: n.nethercote, Assigned: MatsPalmgren_bugz)
References
Details
Attachments
(1 file)
3.79 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
cppcheck says:
> layout/generic/nsBlockFrame.cpp:5098: error: Uninitialized variable: overflowLines
AFAICT it's correct. Or perhaps it's possible that |lineList == &mLines| is always true? I can't tell.
I've marked this as security-sensitive to be cautious, but it may be overkill.
Comment 1•9 years ago
|
||
Looks like this issue (at least, the uninitialized aspect) was introduced with this change for bug 728908: > - nsFrameList overflowFrames; > + FrameLines* overflowLines; [...] > - nsFrameList& frames = lineList == &mLines ? mFrames : overflowFrames; > + nsFrameList& frames = lineList == &mLines ? mFrames : overflowLines->mFrames; https://hg.mozilla.org/mozilla-central/rev/a4095ea6949b#l2.794 Before that change, we could have an empty (but initialized) frame list. Now instead of that, it looks like we have an uninitialized pointer, which we dereference (to grab "mFrames" off of it). Mats, perhaps you can take this?
Assignee | ||
Comment 2•9 years ago
|
||
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=b539dc005423#5037 5037 FrameLines* overflowLines; 5038 nsLineList* lineList = &mLines; So we start out with the condition "lineList = &mLines" being true. 5051 overflowLines = GetOverflowLines(); 5052 lineList = overflowLines ? &overflowLines->mLines : nullptr; Here 'lineList' is assigned something that is != '&mLines', and we initialize 'overflowLines' to a valid value. 5060 lineList = nullptr; 5061 } 5062 } 5063 if (!lineList) { 5064 // Note: defensive code! RFindLineContaining must not return 5065 // false in this case, so if it does... 5066 NS_NOTREACHED("prev sibling not in line list"); 5067 lineList = &mLines; This should never happen, but if it does, we reset 'lineList'. ... no further assigments to 'overflowLines' or 'lineList' ... 5098 nsFrameList& frames = lineList == &mLines ? mFrames : overflowLines->mFrames; 1. 'overflowLines' is only used when 'lineList != &mLines'. 2. 'lineList' is only assigned to something else on line 5052 and 5060. 3. If it was assigned 'nullptr' it is always reset to '&mLines' (5067). Thus, the only way 'lineList == &mLines' is false on line 5098 is when 'lineList = &overflowLines->mLines' occurred on line 5052. In this case, 'overflowLines' has a valid value. The code could be simpler here though...
Flags: needinfo?(mats)
Assignee | ||
Comment 3•9 years ago
|
||
Move 'overflowLines' to an inner scope, and use a nsFrameList* variable in the outer scope instead. This makes it clearer that we only use the pair 'mLines' + 'mFrames', or the pair 'overflowLines->mLines' + 'overflowLines->mFrames' when we find the prev-sibling successfully on the overflow list. https://treeherder.mozilla.org/#/jobs?repo=try&revision=4619b147579a
Assignee: nobody → mats
Attachment #8544025 -
Flags: review?(roc)
Updated•9 years ago
|
Keywords: csectype-uninitialized
Comment 4•9 years ago
|
||
So the variable isn't actually uninitialized? Can this bug be unhidden then Mats?
Keywords: csectype-uninitialized
Assignee | ||
Comment 5•9 years ago
|
||
AFAICT, it's a false positive warning. I wanted to give others a chance to comment before I opened it up, in case I made a mistake.
Summary: Use of uninitialized |overflowLines| in nsBlockFrame::AddFrames() → False positive warning "Use of uninitialized |overflowLines|" in nsBlockFrame::AddFrames()
Attachment #8544025 -
Flags: review?(roc) → review+
Reporter | ||
Comment 6•9 years ago
|
||
Thanks for the simplication, Mats. Definitely feels like the right thing to do given that the current behaviour is so subtle.
Assignee | ||
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d470bfb9e47
Group: core-security
Flags: in-testsuite-
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1d470bfb9e47
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•