Bug 159400 - %TypedArray%.prototype.indexOf is coercing non-integers or non-floats to numbers wrongly
Summary: %TypedArray%.prototype.indexOf is coercing non-integers or non-floats to numb...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
: CVE-2016-4734 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-07-04 00:48 PDT by Caio Lima
Modified: 2016-07-18 12:15 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.05 KB, patch)
2016-07-14 17:26 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (11.30 KB, patch)
2016-07-14 17:42 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (21.60 KB, patch)
2016-07-15 12:19 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2016-07-04 00:48:31 PDT
Current implementation of %TypedArray%.prototype.indexOf is coercing the argument to Array type and generating strange results, which are potential bug generators. Check the following example:

>>> new UInt8Array([0, 1, 2]).indexOf("abc");
0

The return value is 0 because "abc" is being converted to 0. It makes more sense of the result is -1.
Comment 1 Keith Miller 2016-07-14 17:26:48 PDT
Created attachment 283713 [details]
Patch
Comment 2 Keith Miller 2016-07-14 17:42:46 PDT
Created attachment 283718 [details]
Patch
Comment 3 Caio Lima 2016-07-14 17:57:08 PDT
(In reply to comment #1)
> Created attachment 283713 [details]
> Patch

Hey Keith, I took a look in your patch, but I can't comment there, so I am going to point the most important thing I saw =).
The verification:

if (!valueToFind.isNumber())
    return JSValue::encode(jsNumber(-1));

is not enough to implement indexOf because of 3 reasons:

1. It is against spec, because it doesn't perform toInteger. (https://tc39.github.io/ecma262/2016/#sec-array.prototype.indexof), so, if fromIndex is an object, valueOf need to be called. Take a look in this Benjamin's comment in my last patch for %TypedArray%.prototype.includes: https://bugs.webkit.org/show_bug.cgi?id=159385#c38 

2. Values that are numbers will be coerced improperly:
E.g. new Int32Array([1, 2, 3]).indexOf(2.5); // returns 1. Should return -1.

I created a new method in ToNativeFromValue.h with the following signature "bool toNativeFromValue(ExecState* exec, JSValue value, typename Adaptor::Type& result)". This method returns false if it is not possible convert the JSValue to nativeNumber in the Array. So, it already checks cases such as:

1. new UInt8Array([0]).indexOf(NaN); // should return -1. 
2. new  UInt8Array([0]).indexOf(256); // should return -1 because of UInt8 boundaries. 
2. new UInt8Array([2]).indexOf(2.5); // should return -1, because 2.5 is not an element of UInt8Array.
...

There are a lot of cases that need to be considered. The best source of this information is "LayoutTests/js/script-tests/typed-array-includes.js", where almost all ".includes" calls should be replaced to "indexOf" and when result should be false, it shall be -1.

3. NaN cases aren't being considered.

Hope helped =).
Comment 4 Keith Miller 2016-07-14 18:25:41 PDT
(In reply to comment #3)
> (In reply to comment #1)
> > Created attachment 283713 [details]
> > Patch
> 
> Hey Keith, I took a look in your patch, but I can't comment there, so I am
> going to point the most important thing I saw =).
> The verification:
> 
> if (!valueToFind.isNumber())
>     return JSValue::encode(jsNumber(-1));
> 
> is not enough to implement indexOf because of 3 reasons:
> 
> 1. It is against spec, because it doesn't perform toInteger.
> (https://tc39.github.io/ecma262/2016/#sec-array.prototype.indexof), so, if
> fromIndex is an object, valueOf need to be called. Take a look in this
> Benjamin's comment in my last patch for %TypedArray%.prototype.includes:
> https://bugs.webkit.org/show_bug.cgi?id=159385#c38 
> 
> 2. Values that are numbers will be coerced improperly:
> E.g. new Int32Array([1, 2, 3]).indexOf(2.5); // returns 1. Should return -1.
> 
> I created a new method in ToNativeFromValue.h with the following signature
> "bool toNativeFromValue(ExecState* exec, JSValue value, typename
> Adaptor::Type& result)". This method returns false if it is not possible
> convert the JSValue to nativeNumber in the Array. So, it already checks
> cases such as:
> 
> 1. new UInt8Array([0]).indexOf(NaN); // should return -1. 
> 2. new  UInt8Array([0]).indexOf(256); // should return -1 because of UInt8
> boundaries. 
> 2. new UInt8Array([2]).indexOf(2.5); // should return -1, because 2.5 is not
> an element of UInt8Array.
> ...
> 
> There are a lot of cases that need to be considered. The best source of this
> information is "LayoutTests/js/script-tests/typed-array-includes.js", where
> almost all ".includes" calls should be replaced to "indexOf" and when result
> should be false, it shall be -1.
> 
> 3. NaN cases aren't being considered.
> 
> Hope helped =).

Good catch I forgot about those points. I'll fix.
Comment 5 Keith Miller 2016-07-15 12:19:22 PDT
Created attachment 283781 [details]
Patch
Comment 6 Geoffrey Garen 2016-07-15 13:35:39 PDT
Comment on attachment 283781 [details]
Patch

r=me
Comment 7 WebKit Commit Bot 2016-07-15 13:59:42 PDT
Comment on attachment 283781 [details]
Patch

Clearing flags on attachment: 283781

Committed r203297: <http://trac.webkit.org/changeset/203297>
Comment 8 WebKit Commit Bot 2016-07-15 13:59:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Keith Miller 2016-07-18 11:17:15 PDT
*** Bug 159241 has been marked as a duplicate of this bug. ***
Comment 10 David Kilzer (:ddkilzer) 2016-07-18 12:15:55 PDT
<rdar://problem/27324561>