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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Andi [:andi]]

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 1

[Andi [:andi]]

Attachment: MozReview Request: Bug 1239251 - initialize mCommonAncestorWithLastFrame with nullptr in constructor BuildTextRunsScanner. r=[e-mail]

Review commit: https://reviewboard.mozilla.org/r/30639/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30639/

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8707354 [details]
MozReview Request: Bug 1239251 - initialize mCommonAncestorWithLastFrame with nullptr in constructor BuildTextRunsScanner. r=[e-mail]

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.

Comment 3

[Andi [:andi]]

Comment on attachment 8707354 [details]
MozReview Request: Bug 1239251 - initialize mCommonAncestorWithLastFrame with nullptr in constructor BuildTextRunsScanner. r=[e-mail]

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30639/diff/1-2/

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6129a40ddf4

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6129a40ddf4