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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

Attachment: base 'end' off of Elements rather than iter.

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

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Attachment: unit test that fails without patch, succeeds with patch

Here is a unit test.  Obviously, a unit test that depends on data existing beyond the official bounds of the array is implementation-dependent.

Comment 2

[David Ascher (:davida)]

are there any security concerns w/ this bug?

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Er, and fixed on trunk.

Comment 5

[David :Bienvenu]

Comment on attachment 371785 [details] [diff] [review]
base 'end' off of Elements rather than iter.

requesting approval for 1.9.1

Comment 6

[Benjamin Smedberg]

Comment on attachment 371785 [details] [diff] [review]
base 'end' off of Elements rather than iter.

Please land this with the unit test.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

landed on 1.9.1:
fix: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0acd00851e60
unit test: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0ac64f9870d4