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

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

Trunk
mozilla55
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
Created attachment 8848268 [details] [diff] [review]
Add release bounds checking when inserting and replacing

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)

Updated

2 years ago
Assignee: nobody → erahm
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8848276 [details] [diff] [review]
Add release bounds checking when inserting and replacing

Sorry for the churn, missed one.
Attachment #8848276 - Flags: review?(nfroyd)
(Assignee)

Updated

2 years ago
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+
(Assignee)

Comment 5

2 years ago
Created attachment 8848606 [details] [diff] [review]
Add release bounds checking when inserting and replacing

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
(Assignee)

Updated

2 years ago
Attachment #8848276 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
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+
(Assignee)

Comment 8

2 years ago
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
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 10

2 years ago
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?

Updated

2 years ago
status-firefox53: --- → affected
status-firefox54: --- → affected
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+

Comment 12

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/473d3b13cc78
status-firefox54: affected → fixed

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/d8b1e9e5362d
status-firefox53: affected → fixed
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.