Closed Bug 1239251 Opened 4 years ago Closed 4 years ago

[Static Analysis][Uninitialized pointer field] In function BuildTextRunsScanner(..) from nsTextFrame.cpp

Categories

(Core :: Layout, 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 1347687 )

Attachments

(1 file)

The Static Analysis tool Coverity added that mCommonAncestorWithLastFrame, mStartOfLine and mCanStopOnThisLine  are not initialized in the constructor nor in any functions that it calls.

Regarding the scalars mStartOfLine and mCanStopOnThisLine before being used they are initialized by function:

>>  void SetAtStartOfLine() {
>>    mStartOfLine = true;
>>    mCanStopOnThisLine = false;
>>  }

and it's called in the following context:

>>    // Just loop through all the children of the linecontainer ... it's really
>>    // just one line
>>    scanner.SetAtStartOfLine();
>>    scanner.SetCommonAncestorWithLastFrame(nullptr);
>>    for (nsIFrame* child : textRunContainer->PrincipalChildList()) {
>>      scanner.ScanFrame(child);
>>    }

So this part of the checker can be waved.

Regarding the second part of the checker, mCommonAncestorWithLastFrame it's attributed value through function AccumulateRunInfo:

>>  mLastFrame = aFrame;
>>  mCommonAncestorWithLastFrame = aFrame->GetParent();
>>
>>  MappedFlow* mappedFlow = &mMappedFlows[mMappedFlows.Length() - 1];

It's value it's used in function ScanFrame if the following branch is not executed:


>>  // First check if we can extend the current mapped frame block. This is common.
>>  if (mMappedFlows.Length() > 0) {
>>    MappedFlow* mappedFlow = &mMappedFlows[mMappedFlows.Length() - 1];
>>    if (mappedFlow->mEndFrame == aFrame &&
>>        (aFrame->GetStateBits() & NS_FRAME_IS_FLUID_CONTINUATION)) {
>>      NS_ASSERTION(frameType == nsGkAtoms::textFrame,
>>                   "Flow-sibling of a text frame is not a text frame?");
>>
>>      // Don't do this optimization if mLastFrame has a terminal newline...
>>      // it's quite likely preformatted and we might want to end the textrun here.
>>      // This is almost always true:
>>      if (mLastFrame->StyleContext() == aFrame->StyleContext() &&
>>          !HasTerminalNewline(mLastFrame)) {
>>        AccumulateRunInfo(static_cast<nsTextFrame*>(aFrame));
>>        return;
>>      }
>>    }
>>  }

but if this one will be:

>>  // Now see if we can add a new set of frames to the current textrun
>>  if (frameType == nsGkAtoms::textFrame) {
>>    nsTextFrame* frame = static_cast<nsTextFrame*>(aFrame);
>>
>>    if (mLastFrame) {
>>      if (!ContinueTextRunAcrossFrames(mLastFrame, frame)) {
>>        FlushFrames(false, false);
>>      } else {
>>        if (mLastFrame->GetContent() == frame->GetContent()) {
>>          AccumulateRunInfo(frame);
>>          return;
>>        }
>>      }
>>    }
>
>>    MappedFlow* mappedFlow = mMappedFlows.AppendElement();
>>    if (!mappedFlow)
>>      return;
>>
>>    mappedFlow->mStartFrame = frame;
>>    mappedFlow->mAncestorControllingInitialBreak = mCommonAncestorWithLastFrame

I code does not return it can be seen that:
>>  mappedFlow->mAncestorControllingInitialBreak = mCommonAncestorWithLastFrame

So if think it could be possible that mappedFlow->mAncestorControllingInitialBreak would be used with a garbage value thus resulting in an undefined behavior. If you think that this case is impossible i will wave completley this checker.
Comment on attachment 8707354 [details]
MozReview Request: Bug 1239251 - initialize mCommonAncestorWithLastFrame with nullptr in constructor BuildTextRunsScanner. r=roc@ocallahan.org

https://reviewboard.mozilla.org/r/30639/#review27633

It's cleared via
    scanner.SetCommonAncestorWithLastFrame(nullptr);
called from BuildTextRuns. I don't think it's used before that's called.

However it is a bit subtle and there's no harm in initializing it to null in the constructor.
Keywords: checkin-needed
Attachment #8707354 - Attachment description: MozReview Request: Bug 1239251 - initialize mCommonAncestorWithLastFrame with nullptr in constructor BuildTextRunsScanner. r?roc@ocallahan.org → MozReview Request: Bug 1239251 - initialize mCommonAncestorWithLastFrame with nullptr in constructor BuildTextRunsScanner. r=roc@ocallahan.org
Comment on attachment 8707354 [details]
MozReview Request: Bug 1239251 - initialize mCommonAncestorWithLastFrame with nullptr in constructor BuildTextRunsScanner. r=roc@ocallahan.org

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30639/diff/1-2/
https://hg.mozilla.org/mozilla-central/rev/b6129a40ddf4
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.