Closed Bug 1236920 Opened 4 years ago Closed 4 years ago

[Static Analysis][Dereference before null check] In function nsAbsoluteContainingBlock::ReflowAbsoluteFrame from nsAbsoluteContainingBlock.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 1346168)

Attachments

(1 file)

The Static Analysis tool Coverity added that variable aKidFrame is dereferenced before it's null checked:

dereference:

>>  WritingMode wm = aKidFrame->GetWritingMode();
>>  LogicalSize logicalCBSize(wm, aContainingBlock.Size());

null check:

>>    if (aKidFrame) {
>>      nsAutoString name;
>>      aKidFrame->GetFrameName(name);
>>      printf("%s ", NS_LossyConvertUTF16toASCII(name).get());
>>    }

As we can see from the calling of function ReflowAbsoluteFrame, variable aKidFrame is always passed a valid pointer:

>>  for (kidFrame = mAbsoluteFrames.FirstChild(); kidFrame; kidFrame = kidFrame->GetNextSibling()) {
>>    bool kidNeedsReflow = reflowAll || NS_SUBTREE_DIRTY(kidFrame) ||
>>      FrameDependsOnContainer(kidFrame,
>>                              !!(aFlags & AbsPosReflowFlags::eCBWidthChanged),
>>                              !!(aFlags & AbsPosReflowFlags::eCBHeightChanged));
>>    if (kidNeedsReflow && !aPresContext->HasPendingInterrupt()) {
>>      // Reflow the frame
>>      nsReflowStatus  kidStatus = NS_FRAME_COMPLETE;
>>      const nsRect& cb = isGrid ? nsGridContainerFrame::GridItemCB(kidFrame)
>>                                : aContainingBlock;
>>      ReflowAbsoluteFrame(aDelegatingFrame, aPresContext, aReflowState, cb,
>>                          aFlags, kidFrame, kidStatus, aOverflowAreas); 

From the above context kidFrame is passed as aKidFrame in ReflowAbsoluteFrame and we can see that function gets called while kidFrame is valid, thus making useless the null check for on aKidFrame. The checks are done only on Debug but in this way we silence Coverity.
Attached patch Bug 1236920.diffSplinter Review
Attachment #8704147 - Flags: review?(matt.woodrow)
Attachment #8704147 - Flags: review?(matt.woodrow) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e74d457b8b11
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.