Closed Bug 1117596 Opened 5 years ago Closed 5 years ago

False positive warning "Use of uninitialized |overflowLines|" in nsBlockFrame::AddFrames()

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: njn, Assigned: mats)

References

Details

Attachments

(1 file)

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.
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?
Depends on: 728908
Flags: needinfo?(mats)
OS: Linux → All
Hardware: x86_64 → All
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)
Attached patch fixSplinter Review
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)
So the variable isn't actually uninitialized?  Can this bug be unhidden then Mats?
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()
Thanks for the simplication, Mats. Definitely feels like the right thing to do given that the current behaviour is so subtle.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d470bfb9e47
Group: core-security
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/1d470bfb9e47
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.