Closed Bug 487551 Opened 13 years ago Closed 13 years ago

nsTArray.IndexOf may scan beyond array bounds for non-zero start argument

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: asuth, Assigned: asuth)

References

Details

(Keywords: fixed1.9.1)

Attachments

(2 files)

The iterator and end-iterator are initialized like so:
const elem_type* iter = Elements() + start, *end = iter + Length();

Since the 'end' iterator is using Length(), it probably wants to be using Elements() too:
const elem_type* iter = Elements() + start, *end = Elements() + Length();

Ideally the fix could hit 1.9.1 as this causes Thunderbird to crash like described in (core/xpcom) bug 455216.  We can work around the issue, but it would be nice not to have to.

unit test as next patch
Attachment #371785 - Flags: review?(benjamin)
Flags: wanted1.9.1?
Here is a unit test.  Obviously, a unit test that depends on data existing beyond the official bounds of the array is implementation-dependent.
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Attachment #371786 - Flags: review?(benjamin)
Blocks: 482195
are there any security concerns w/ this bug?
Whiteboard: [tb3needs]
Attachment #371785 - Flags: review?(benjamin) → review+
Attachment #371786 - Flags: review?(benjamin) → review+
These landed in mozilla-central on friday, but I forgot to hit submit:

fix pushed: http://hg.mozilla.org/mozilla-central/rev/ed0e7eabef8f
unit test pushed: http://hg.mozilla.org/mozilla-central/rev/730004d7878c

It does not appear that I have the privileges required to request approval1.9.1.  davida/dmose/bienvenu, maybe you can do that?

(In reply to comment #2)
> are there any security concerns w/ this bug?

I doubt there are exploitable security impacts from this bug; I would expect mainly potential crashers and memory corruption in cases where the indexOf is speculative.

However, it's a ridiculously safe fix for an arguably serious correctness issue, which is why it should be allowed in 1.9.1.
Flags: in-testsuite+
Er, and fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment on attachment 371785 [details] [diff] [review]
base 'end' off of Elements rather than iter.

requesting approval for 1.9.1
Attachment #371785 - Flags: approval1.9.1?
Attachment #371785 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 371785 [details] [diff] [review]
base 'end' off of Elements rather than iter.

Please land this with the unit test.
You need to log in before you can comment on or make changes to this bug.