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

RESOLVED FIXED in mozilla37



3 years ago
3 years ago


(Reporter: njn, Assigned: mats)


(Blocks: 1 bug)

Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(1 attachment)



3 years ago
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;

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

Comment 2

3 years ago

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)

Comment 3

3 years ago
Created attachment 8544025 [details] [diff] [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.

Assignee: nobody → mats
Attachment #8544025 - Flags: review?(roc)
Keywords: csectype-uninitialized
So the variable isn't actually uninitialized?  Can this bug be unhidden then Mats?
Keywords: csectype-uninitialized

Comment 5

3 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()

Comment 6

3 years ago
Thanks for the simplication, Mats. Definitely feels like the right thing to do given that the current behaviour is so subtle.

Comment 7

3 years ago
Group: core-security
Flags: in-testsuite-
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.