Bug 44830 – In Array's prototype functions we're incorrectly handing large index values

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gavin Barraclough]

We are in places casting doubles to unsigneds, and unsigneds to ints, without always check that the result is within bounds.  This is problematic in the case of double-to-unsigned conversion because we should be saturating to array length.

Also, the error return value from Array.splice should be [], not undefined.

I don't see any security concerns here.  These methods are spec'ed in such a way that they can be applied to non Array objects, so In all cases the (potentially bogus) indices are being passed to functions that will safely check accesses are within bounds.

[Gavin Barraclough]

Created attachment 65840 [details]
The patch

[Gavin Barraclough]

*** Bug 38356 has been marked as a duplicate of this bug. ***

[Oliver Hunt]

Comment on attachment 65840 [details]
The patch

r=me

[Darin Adler]

What’s the performance impact of these changes?

[Gavin Barraclough]

Fixed in r66318.

[Gavin Barraclough]

(In reply to comment #4)
> What’s the performance impact of these changes?

Short answer, none.

Most of the benchmarks we track don't call these methods anyway.  For the most part the change is just to change a type (int vs unsigned), and should result in approx. the same code.  The two functional changes are to make Array.splice properly saturate large values - which is one extra integer compare instruction in an expensive function (allocates a JSArray).  Also changes return value if called with invalid parameters, which I would not expect to show up in any real code, and Michael has done work lately to reduce the cost of allocating unused Array objects which would also help mitigate this.

No change on SunSpider.