Closed Bug 1308208 Opened 3 years ago Closed 3 years ago

[Static Analysis][Dereference after null check] In function GridLines::SetLineInfo

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1373428)

Attachments

(1 file)

The Static Analysis tool Coverity detected that |aLineInfo| is null checked and outside of the scope of the checked is dereferenced causing a possible null pointer deference. 

Check:

>>     if (aLineInfo) {
>>        lineNames = aLineInfo->mNames.SafeElementAt(i, nsTArray<nsString>());
>>      }

Dereference:

>>      if (i >= aTrackInfo->mRepeatFirstTrack &&
>>          repeatIndex < numRepeatTracks) {
>>        numAddedLines += AppendRemovedAutoFits(aTrackInfo,
>>                                               aLineInfo,
>>                                               lastTrackEdge,
>>                                               repeatIndex,
>>                                               numRepeatTracks,
>>                                               lineNames);
>>      }

I think the null check is useless since all path that call GridLines::SetLineInfo guarantee that |aLineInfo| is not-null.

For example:

>>  const ComputedGridLineInfo* rowLineInfo =
>>    aFrame->GetComputedTemplateRowLines();
>>  mRows->SetTrackInfo(rowTrackInfo);
>>  mRows->SetLineInfo(rowTrackInfo, rowLineInfo, mAreas, true);
>>
>>  const ComputedGridTrackInfo* columnTrackInfo =
>>    aFrame->GetComputedTemplateColumns();
>>  const ComputedGridLineInfo* columnLineInfo =
>>    aFrame->GetComputedTemplateColumnLines();
>>  mCols->SetTrackInfo(columnTrackInfo);
>>  mCols->SetLineInfo(columnTrackInfo, columnLineInfo, mAreas, false);

Functions GetComputedTemplateRowLines and GetComputedTemplateColumnLines guard the return values with assert.
Comment on attachment 8798438 [details]
Bug 1308208 - remove redundant null check from GridLines::SetLineInfo.

https://reviewboard.mozilla.org/r/83940/#review82488

::: dom/grid/GridLines.cpp
(Diff revision 1)
>        startOfNextTrack = (i < aTrackInfo->mEndFragmentTrack) ?
>                           aTrackInfo->mPositions[i] :
>                           lastTrackEdge;
>  
>        nsTArray<nsString> lineNames;
> -      if (aLineInfo) {

MOZ_ASSERT(aLineInfo); at the beginning of the method.
Comment on attachment 8798438 [details]
Bug 1308208 - remove redundant null check from GridLines::SetLineInfo.

https://reviewboard.mozilla.org/r/83940/#review82490
Attachment #8798438 - Flags: review?(amarchesini) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d63144fb5dc
remove redundant null check from GridLines::SetLineInfo. r=baku
https://hg.mozilla.org/mozilla-central/rev/6d63144fb5dc
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.