Closed Bug 1348123 Opened 3 years ago Closed 3 years ago

Add non-debug mode bounds checking to nsTArray::InsertElementAt and nsTArray::ReplaceElementAt

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: erahm, Assigned: erahm)

Details

Attachments

(1 file, 2 obsolete files)

Similar to bug 1159244 we should check bounds when inserting and replacing at a specific location. We probably don't have to worry about a perf hit here as inserting anywhere but the end isn't expected to be terribly fast.

Mainly I'm thinking of:
  - |RelpaceElementsAt| [1]
  - |InsertElementsAt| [2]
  - |InsertElementAt| [3]

[1] http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/xpcom/ds/nsTArray.h#1962
[2] http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/xpcom/ds/nsTArray.h#2020
[3] http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/xpcom/ds/nsTArray.h#2042
This adds release bounds checking to ReplaceElementsAt, InsertElementAt, and
InsertElementsAt to make sure the insertion point is within the current array
bounds.

MozReview-Commit-ID: 1pFr8LuOROI
Attachment #8848268 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Sorry for the churn, missed one.
Attachment #8848276 - Flags: review?(nfroyd)
Attachment #8848268 - Attachment is obsolete: true
Attachment #8848268 - Flags: review?(nfroyd)
Comment on attachment 8848276 [details] [diff] [review]
Add release bounds checking when inserting and replacing

Review of attachment 8848276 [details] [diff] [review]:
-----------------------------------------------------------------

Will be fascinating to see if anything turns up from these.
Attachment #8848276 - Flags: review?(nfroyd) → review+
This adds release bounds checking to ReplaceElementsAt, InsertElementAt, and
InsertElementsAt to make sure the insertion point is within the current array
bounds.

MozReview-Commit-ID: 1pFr8LuOROI
Attachment #8848276 - Attachment is obsolete: true
Comment on attachment 8848606 [details] [diff] [review]
Add release bounds checking when inserting and replacing

Updated the |>=| checks for InsertAt to be |>|; inserting at the end of the array is legit. Carrying forward r+ from IRC.
Attachment #8848606 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6b4d237ef6732d19240dde3b4fa9ac1d1d61aa
Bug 1348123 - Add release bounds checking when inserting and replacing. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/0e6b4d237ef6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8848606 [details] [diff] [review]
Add release bounds checking when inserting and replacing

> Approval Request Comment
> [Feature/Bug causing the regression]:

This is a diagnostic improvement, it enforces bounds checking nsTArray insertion functions.

> [User impact if declined]:

Hard to track down crashes.

> [Is this code covered by automated tests?]:

These code paths are thoroughly tested.

> [Has the fix been verified in Nightly?]:

Baked on nightly for a few days.

> [Needs manual test from QE? If yes, steps to reproduce]: 

No.

> [List of other uplifts needed for the feature/fix]:

N/A

> [Is the change risky?]:

Low risk.

> [Why is the change risky/not risky?]:

This just flags erroneous behavior earlier and should give us better crash reports.

> [String changes made/needed]:

N/A
Attachment #8848606 - Flags: approval-mozilla-beta?
Attachment #8848606 - Flags: approval-mozilla-aurora?
Comment on attachment 8848606 [details] [diff] [review]
Add release bounds checking when inserting and replacing

This is a diagnostic improvement. We can take it. Aurora54+ & Beta53+.
Attachment #8848606 - Flags: approval-mozilla-beta?
Attachment #8848606 - Flags: approval-mozilla-beta+
Attachment #8848606 - Flags: approval-mozilla-aurora?
Attachment #8848606 - Flags: approval-mozilla-aurora+
Setting qe-verify- based on comment 10.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.