JSON.parse incorrectly handles array proxies
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 8 Oct 2019 21:23:14 +0000 (21:23 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 8 Oct 2019 21:23:14 +0000 (21:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199292

Patch by Alexey Shvayka <shvaikalesh@gmail.com> on 2019-10-08
Reviewed by Saam Barati.

JSTests:

* microbenchmarks/json-parse-array-reviver-same-value.js: Added.
* microbenchmarks/json-parse-array-reviver.js: Added.
* microbenchmarks/json-parse-object-reviver-same-value.js: Added.
* microbenchmarks/json-parse-object-reviver.js: Added.
* stress/json-parse-reviver-array-proxy.js: Added.
* stress/json-parse-reviver-revoked-proxy.js: Added.
* test262/expectations.yaml: Mark 6 test cases as passing.

Source/JavaScriptCore:

1. Use isArray to correctly detect proxied arrays.
2. Make "length" lookup observable to array proxies and handle exceptions.

* runtime/JSONObject.cpp:
(JSC::Walker::walk):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@250860 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/microbenchmarks/json-parse-array-reviver-same-value.js [new file with mode: 0644]
JSTests/microbenchmarks/json-parse-array-reviver.js [new file with mode: 0644]
JSTests/microbenchmarks/json-parse-object-reviver-same-value.js [new file with mode: 0644]
JSTests/microbenchmarks/json-parse-object-reviver.js [new file with mode: 0644]
JSTests/stress/json-parse-reviver-array-proxy.js [new file with mode: 0644]
JSTests/stress/json-parse-reviver-revoked-proxy.js [new file with mode: 0644]
JSTests/test262/expectations.yaml
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/JSONObject.cpp

index 991c099..462ba63 100644 (file)
@@ -1,3 +1,18 @@
+2019-10-08  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        JSON.parse incorrectly handles array proxies
+        https://bugs.webkit.org/show_bug.cgi?id=199292
+
+        Reviewed by Saam Barati.
+
+        * microbenchmarks/json-parse-array-reviver-same-value.js: Added.
+        * microbenchmarks/json-parse-array-reviver.js: Added.
+        * microbenchmarks/json-parse-object-reviver-same-value.js: Added.
+        * microbenchmarks/json-parse-object-reviver.js: Added.
+        * stress/json-parse-reviver-array-proxy.js: Added.
+        * stress/json-parse-reviver-revoked-proxy.js: Added.
+        * test262/expectations.yaml: Mark 6 test cases as passing.
+
 2019-10-08  Ross Kirsling  <ross.kirsling@sony.com>
 
         Update test262 (2019.10.08).
 2019-10-08  Ross Kirsling  <ross.kirsling@sony.com>
 
         Update test262 (2019.10.08).
diff --git a/JSTests/microbenchmarks/json-parse-array-reviver-same-value.js b/JSTests/microbenchmarks/json-parse-array-reviver-same-value.js
new file mode 100644 (file)
index 0000000..13956af
--- /dev/null
@@ -0,0 +1,3 @@
+const json = JSON.stringify([0,1,2,3,4,5,6,7,8,9]);
+for (let i = 0; i < 1e5; ++i)
+    JSON.parse(json, (_key, val) => val);
diff --git a/JSTests/microbenchmarks/json-parse-array-reviver.js b/JSTests/microbenchmarks/json-parse-array-reviver.js
new file mode 100644 (file)
index 0000000..80e3635
--- /dev/null
@@ -0,0 +1,3 @@
+const json = JSON.stringify([0,1,2,3,4,5,6,7,8,9]);
+for (let i = 0; i < 1e5; ++i)
+    JSON.parse(json, (_key, val) => val + 1);
diff --git a/JSTests/microbenchmarks/json-parse-object-reviver-same-value.js b/JSTests/microbenchmarks/json-parse-object-reviver-same-value.js
new file mode 100644 (file)
index 0000000..1eee2e6
--- /dev/null
@@ -0,0 +1,3 @@
+const json = JSON.stringify({a:0,b:1,c:2,d:3,e:4,f:5,g:6,h:7,i:8,j:9});
+for (let i = 0; i < 1e5; ++i)
+    JSON.parse(json, (_key, val) => val);
diff --git a/JSTests/microbenchmarks/json-parse-object-reviver.js b/JSTests/microbenchmarks/json-parse-object-reviver.js
new file mode 100644 (file)
index 0000000..2edf3fe
--- /dev/null
@@ -0,0 +1,3 @@
+const json = JSON.stringify({a:0,b:1,c:2,d:3,e:4,f:5,g:6,h:7,i:8,j:9});
+for (let i = 0; i < 1e5; ++i)
+    JSON.parse(json, (_key, val) => val + 1);
diff --git a/JSTests/stress/json-parse-reviver-array-proxy.js b/JSTests/stress/json-parse-reviver-array-proxy.js
new file mode 100644 (file)
index 0000000..99f97f7
--- /dev/null
@@ -0,0 +1,28 @@
+function shouldBe(actual, expected) {
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+const json = '{"a": 1, "b": 2}';
+
+for (let i = 1; i < 10000; i++) {
+    let keys = [];
+    let proxy = new Proxy([2, 3], {
+        get: function(target, key) {
+            keys.push(key);
+            return target[key];
+        },
+        ownKeys: function() {
+            throw new Error('[[OwnPropertyKeys]] should not be called');
+        },
+    });
+
+    let result = JSON.parse(json, function(key, value) {
+        if (key === 'a')
+            this.b = proxy;
+        return value;
+    });
+
+    shouldBe(keys.toString(), 'length,0,1');
+    shouldBe(JSON.stringify(result), '{"a":1,"b":[2,3]}');
+}
diff --git a/JSTests/stress/json-parse-reviver-revoked-proxy.js b/JSTests/stress/json-parse-reviver-revoked-proxy.js
new file mode 100644 (file)
index 0000000..9cb88db
--- /dev/null
@@ -0,0 +1,48 @@
+function shouldBe(actual, expected) {
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+function revivedObjProxy(key, value) {
+    if (key === 'a') {
+        let {proxy, revoke} = Proxy.revocable({}, {});
+        revoke();
+        this.b = proxy;
+    }
+
+    return value;
+}
+
+function revivedArrProxy(key, value) {
+    if (key === '0') {
+        let {proxy, revoke} = Proxy.revocable([], {});
+        revoke();
+        this[1] = proxy;
+    }
+
+    return value;
+}
+
+const objJSON = '{"a": 1, "b": 2}';
+const arrJSON = '[3, 4]';
+const isArrayError = 'TypeError: Array.isArray cannot be called on a Proxy that has been revoked';
+
+for (let i = 1; i < 10000; i++) {
+    let error;
+    try {
+        JSON.parse(objJSON, revivedObjProxy);
+    } catch (e) {
+        error = e;
+    }
+    shouldBe(error.toString(), isArrayError);
+}
+
+for (let i = 1; i < 10000; i++) {
+    let error;
+    try {
+        JSON.parse(arrJSON, revivedArrProxy);
+    } catch (e) {
+        error = e;
+    }
+    shouldBe(error.toString(), isArrayError);
+}
index 449eedf..c45d462 100644 (file)
@@ -1206,15 +1206,6 @@ test/built-ins/Function/prototype/toString/unicode.js:
 test/built-ins/GeneratorFunction/proto-from-ctor-realm.js:
   default: 'Test262Error: Expected SameValue(«[object GeneratorFunction]», Â«[object GeneratorFunction]») to be true'
   strict mode: 'Test262Error: Expected SameValue(«[object GeneratorFunction]», Â«[object GeneratorFunction]») to be true'
 test/built-ins/GeneratorFunction/proto-from-ctor-realm.js:
   default: 'Test262Error: Expected SameValue(«[object GeneratorFunction]», Â«[object GeneratorFunction]») to be true'
   strict mode: 'Test262Error: Expected SameValue(«[object GeneratorFunction]», Â«[object GeneratorFunction]») to be true'
-test/built-ins/JSON/parse/revived-proxy.js:
-  default: 'Test262Error: proxy for array Expected SameValue(«true», Â«false») to be true'
-  strict mode: 'Test262Error: proxy for array Expected SameValue(«true», Â«false») to be true'
-test/built-ins/JSON/parse/reviver-array-length-coerce-err.js:
-  default: 'Test262Error: Expected a Test262Error to be thrown but no exception was thrown at all'
-  strict mode: 'Test262Error: Expected a Test262Error to be thrown but no exception was thrown at all'
-test/built-ins/JSON/parse/reviver-array-length-get-err.js:
-  default: 'Test262Error: Expected a Test262Error to be thrown but no exception was thrown at all'
-  strict mode: 'Test262Error: Expected a Test262Error to be thrown but no exception was thrown at all'
 test/built-ins/Map/proto-from-ctor-realm.js:
   default: 'Test262Error: Expected SameValue(«[object Map]», Â«[object Map]») to be true'
   strict mode: 'Test262Error: Expected SameValue(«[object Map]», Â«[object Map]») to be true'
 test/built-ins/Map/proto-from-ctor-realm.js:
   default: 'Test262Error: Expected SameValue(«[object Map]», Â«[object Map]») to be true'
   strict mode: 'Test262Error: Expected SameValue(«[object Map]», Â«[object Map]») to be true'
index 6519270..debe513 100644 (file)
@@ -1,3 +1,16 @@
+2019-10-08  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        JSON.parse incorrectly handles array proxies
+        https://bugs.webkit.org/show_bug.cgi?id=199292
+
+        Reviewed by Saam Barati.
+
+        1. Use isArray to correctly detect proxied arrays.
+        2. Make "length" lookup observable to array proxies and handle exceptions.
+
+        * runtime/JSONObject.cpp:
+        (JSC::Walker::walk):
+
 2019-10-08  Adrian Perez de Castro  <aperez@igalia.com>
 
         [GTK][WPE] Fix non-unified builds after r250486
 2019-10-08  Adrian Perez de Castro  <aperez@igalia.com>
 
         [GTK][WPE] Fix non-unified builds after r250486
index 774c5c2..07c511c 100644 (file)
@@ -32,6 +32,7 @@
 #include "Error.h"
 #include "ExceptionHelpers.h"
 #include "JSArray.h"
 #include "Error.h"
 #include "ExceptionHelpers.h"
 #include "JSArray.h"
+#include "JSArrayInlines.h"
 #include "JSGlobalObject.h"
 #include "LiteralParser.h"
 #include "Lookup.h"
 #include "JSGlobalObject.h"
 #include "LiteralParser.h"
 #include "Lookup.h"
@@ -663,19 +664,21 @@ NEVER_INLINE JSValue Walker::walk(JSValue unfiltered)
             arrayStartState:
             case ArrayStartState: {
                 ASSERT(inValue.isObject());
             arrayStartState:
             case ArrayStartState: {
                 ASSERT(inValue.isObject());
-                ASSERT(asObject(inValue)->inherits<JSArray>(vm));
+                ASSERT(isJSArray(inValue) || inValue.inherits<ProxyObject>(vm));
                 if (markedStack.size() > maximumFilterRecursion)
                     return throwStackOverflowError(m_exec, scope);
 
                 if (markedStack.size() > maximumFilterRecursion)
                     return throwStackOverflowError(m_exec, scope);
 
-                JSArray* array = asArray(inValue);
+                JSObject* array = asObject(inValue);
                 markedStack.appendWithCrashOnOverflow(array);
                 markedStack.appendWithCrashOnOverflow(array);
-                arrayLengthStack.append(array->length());
+                unsigned length = toLength(m_exec, array);
+                RETURN_IF_EXCEPTION(scope, { });
+                arrayLengthStack.append(length);
                 indexStack.append(0);
             }
             arrayStartVisitMember:
             FALLTHROUGH;
             case ArrayStartVisitMember: {
                 indexStack.append(0);
             }
             arrayStartVisitMember:
             FALLTHROUGH;
             case ArrayStartVisitMember: {
-                JSArray* array = jsCast<JSArray*>(markedStack.last());
+                JSObject* array = asObject(markedStack.last());
                 uint32_t index = indexStack.last();
                 unsigned arrayLength = arrayLengthStack.last();
                 if (index == arrayLength) {
                 uint32_t index = indexStack.last();
                 unsigned arrayLength = arrayLengthStack.last();
                 if (index == arrayLength) {
@@ -704,7 +707,7 @@ NEVER_INLINE JSValue Walker::walk(JSValue unfiltered)
                 FALLTHROUGH;
             }
             case ArrayEndVisitMember: {
                 FALLTHROUGH;
             }
             case ArrayEndVisitMember: {
-                JSArray* array = jsCast<JSArray*>(markedStack.last());
+                JSObject* array = asObject(markedStack.last());
                 JSValue filteredValue = callReviver(array, jsString(vm, String::number(indexStack.last())), outValue);
                 RETURN_IF_EXCEPTION(scope, { });
                 if (filteredValue.isUndefined())
                 JSValue filteredValue = callReviver(array, jsString(vm, String::number(indexStack.last())), outValue);
                 RETURN_IF_EXCEPTION(scope, { });
                 if (filteredValue.isUndefined())
@@ -718,7 +721,7 @@ NEVER_INLINE JSValue Walker::walk(JSValue unfiltered)
             objectStartState:
             case ObjectStartState: {
                 ASSERT(inValue.isObject());
             objectStartState:
             case ObjectStartState: {
                 ASSERT(inValue.isObject());
-                ASSERT(!asObject(inValue)->inherits<JSArray>(vm));
+                ASSERT(!isJSArray(inValue));
                 if (markedStack.size() > maximumFilterRecursion)
                     return throwStackOverflowError(m_exec, scope);
 
                 if (markedStack.size() > maximumFilterRecursion)
                     return throwStackOverflowError(m_exec, scope);
 
@@ -778,8 +781,9 @@ NEVER_INLINE JSValue Walker::walk(JSValue unfiltered)
                     outValue = inValue;
                     break;
                 }
                     outValue = inValue;
                     break;
                 }
-                JSObject* object = asObject(inValue);
-                if (object->inherits<JSArray>(vm))
+                bool valueIsArray = isArray(m_exec, inValue);
+                RETURN_IF_EXCEPTION(scope, { });
+                if (valueIsArray)
                     goto arrayStartState;
                 goto objectStartState;
         }
                     goto arrayStartState;
                 goto objectStartState;
         }