1345352 - Use of iterator past end of container in nsTableFrame::AddDeletedRowIndex()

Status: RESOLVED INVALID

(Core :: Layout: Tables, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

** CID 1401685:  API usage errors  (INVALIDATE_ITERATOR)
/layout/tables/nsTableFrame.cpp: 1017 in nsTableFrame::AddDeletedRowIndex(int)()


________________________________________________________________________________________________________
*** CID 1401685:  API usage errors  (INVALIDATE_ITERATOR)
/layout/tables/nsTableFrame.cpp: 1017 in nsTableFrame::AddDeletedRowIndex(int)()
1011           // merge current index with smaller and greater range as they are consecutive
1012           smallerIter->second = greaterIter->second;
1013           mDeletedRowIndexRanges.erase(greaterIter);
1014         }
1015         else {
1016           // add aDeletedRowStoredIndex in the smaller range as it is consecutive
>>>     CID 1401685:  API usage errors  (INVALIDATE_ITERATOR)
>>>     Dereferencing iterator "smallerIter" though it is already past the end of its container.
1017           smallerIter->second = aDeletedRowStoredIndex;
1018         }
1019       } else if (greaterIter != mDeletedRowIndexRanges.end() &&
1020                  greaterIter->first == aDeletedRowStoredIndex + 1) {
1021         // add aDeletedRowStoredIndex in the greater range as it is consecutive
1022         mDeletedRowIndexRanges.insert(std::pair<int32_t, int32_t>

Comment 1

[Mats Palmgren (inactive)]

Looks like this code was added in bug 1285874.

Comment 2

[Neerja Pancholi[:neerja]]

(In reply to Mats Palmgren (:mats) from comment #1)
> Looks like this code was added in bug 1285874.

Thanks Mats, I'll take a look. 

:jesup, can you please tell me how you were able to reproduce this? Thanks!

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

> :jesup, can you please tell me how you were able to reproduce this? Thanks!

This is from static analysis - Coverity looked at the code and says this will (or may) happen.

Comment 4

[Neerja Pancholi[:neerja]]

(In reply to Randell Jesup [:jesup] from comment #3)
> > :jesup, can you please tell me how you were able to reproduce this? Thanks!
> 
> This is from static analysis - Coverity looked at the code and says this
> will (or may) happen.

Cool, thanks :jesup.

Comment 5

[Nathan Froyd [:froydnj]]

[Tracking Requested - why for this release]: misusing APIs can have serious consequences.

Comment 6

[Nathan Froyd [:froydnj]]

Neerja, do you have an estimate of when this will be done?

Comment 7

[Neerja Pancholi[:neerja]]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> Neerja, do you have an estimate of when this will be done?

Hi Nathan, I'll fix it this week. 
Thanks!

Comment 8

[Marcia Knous [:marcia]]

Tracking 54/55+ for the reason in Comment 5.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Neerja Pancholi[:neerja] from comment #7)
> (In reply to Nathan Froyd [:froydnj] from comment #6)
> > Neerja, do you have an estimate of when this will be done?
> 
> Hi Nathan, I'll fix it this week. 
> Thanks!

Gentle ping on this bug, since we've now moved 54 to Beta.

Comment 10

[Neerja Pancholi[:neerja]]

(In reply to Nathan Froyd [:froydnj] from comment #9)
> (In reply to Neerja Pancholi[:neerja] from comment #7)
> > (In reply to Nathan Froyd [:froydnj] from comment #6)
> > > Neerja, do you have an estimate of when this will be done?
> > 
> > Hi Nathan, I'll fix it this week. 
> > Thanks!
> 
> Gentle ping on this bug, since we've now moved 54 to Beta.

Hi Nathan,
Sorry, I got distracted with something else. I'm keeping your needinfo as a reminder and I'll clear it when I'm finished fixing this which will be this week. 
Thanks!

Comment 11

[Neerja Pancholi[:neerja]]

After looking at this I can't see a way in which "smallerItr" could ever be out of bounds because of this check: https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1006 and also that the list can never be empty because of: https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#982
(:dholbert helped me cross check this in case I was missing something so thank you!)

I would suggest that this be closed as invalid. Or if someone could point me to what might be making Coverity flag this then I might be able to refactor it to remove the error? What do you think :froydnj and :jesup? 

Some possible suggestions by dholbert as to why Coverity might be complaining -
1. It doesn't know that the list can never be empty because of: https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#982
2. The equality for greaterItr and smallerItr (https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1005) and then a check on greaterItr to not be end of list (https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1011) might make Coverity think that smallerIter here could be end of list?

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

If you're certain it's invalid, go ahead. (or mark it invalid and add a comment about it in the code -- "smallerItr can't be out of bounds because XXXX" and/or add a debug assertion if it's not a hot path

Comment 13

[Nathan Froyd [:froydnj]]

Yeah, after staring at this, I'm not sure why Coverity thinks it can be beyond the *end* of the container--unless the error message is bad, and they meant before the *start* of the container.  If the access being flagged is beyond the end, then surely the access to smallerIter that dominates this use is *also* beyond the end...

Debug assertions might encourage Coverity to understand the code better, but flagging it in Coverity's database itself is also a reasonable option.

Comment 14

[Nathan Froyd [:froydnj]]

Thanks for looking at this!

Comment 15

[Neerja Pancholi[:neerja]]

Thanks jesup and froydnj!
I'm closing this bug as invalid and I've created another bug to add a comment and an assertion to further clarify why smallerItr cannot be out of bounds here.

Comment 16

[Gerry Chang [:gchang]]

Per comment #15, mark 54/55 won't fix.