Bug 199292 - JSON.parse incorrectly handles array proxies
Summary: JSON.parse incorrectly handles array proxies
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Minor
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-27 15:24 PDT by Alexey Shvayka
Modified: 2019-10-08 14:24 PDT (History)
10 users (show)

See Also:


Attachments
Patch (5.62 KB, patch)
2019-06-27 15:34 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (8.26 KB, patch)
2019-07-05 15:19 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (8.30 KB, patch)
2019-08-15 15:16 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (10.28 KB, patch)
2019-09-02 15:20 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (10.45 KB, patch)
2019-10-08 12:32 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Alexey Shvayka 2019-06-27 15:34:57 PDT
Created attachment 373059 [details]
Patch
Comment 2 Yusuke Suzuki 2019-07-02 14:26:48 PDT
Comment on attachment 373059 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373059&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        1. Use isArray to correctly detect proxied arrays.
> +        2. Make "length" lookup observable to array proxies and handle exceptions.

Let's add each test in addition to test262 to ensure this behavior.

> Source/JavaScriptCore/runtime/JSONObject.cpp:675
> +                ASSERT(isArray(m_exec, inValue));
>                  if (markedStack.size() > maximumFilterRecursion)
>                      return throwStackOverflowError(m_exec, scope);
>  
> -                JSArray* array = asArray(inValue);
> +                auto array = asObject(inValue);
>                  markedStack.appendWithCrashOnOverflow(array);
> -                arrayLengthStack.append(array->length());
> +                unsigned length = isJSArray(array)
> +                    ? asArray(array)->length()
> +                    : array->get(m_exec, vm.propertyNames->length).toUInt32(m_exec);
> +                RETURN_IF_EXCEPTION(scope, { });
> +                arrayLengthStack.append(length);

`isArray` is user-observable, side-effect operations. When we encounter the revoked Proxy, then we throw an error.
So,

1. When `isArray` is used, we need to do error-handling correctly.
2. Since this error is observable (like, throwing an error before/after the other operations, which can be observable to users), when calling `isArray` becomes important.

Is this `isArray()` call specified in the spec?
Comment 3 Alexey Shvayka 2019-07-05 15:19:44 PDT
Created attachment 373546 [details]
Patch

Remove isArray from ASSERTs, add stress tests for 1, 2, and isArray order.
Comment 4 Alexey Shvayka 2019-07-05 15:29:36 PDT
(In reply to Yusuke Suzuki from comment #2)
> Comment on attachment 373059 [details]
> Patch
> 
> 1. When `isArray` is used, we need to do error-handling correctly.
> 2. Since this error is observable (like, throwing an error before/after the
> other operations, which can be observable to users), when calling `isArray`
> becomes important.
> 
> Is this `isArray()` call specified in the spec?

Nice catch, thanks. I've replaced ASSERTs with comments and added error-handling for isArray call in StateUnknown (that one is per spec).
Comment 5 Alexey Shvayka 2019-08-15 15:16:09 PDT
Created attachment 376429 [details]
Patch

Replace comments with real ASSERTs, use toLength.
Comment 6 Saam Barati 2019-08-30 15:37:58 PDT
Comment on attachment 376429 [details]
Patch

nice. Can you verify this doesn't make JSON parsing slower?
Comment 7 Alexey Shvayka 2019-09-02 15:20:47 PDT
Created attachment 377862 [details]
Patch

Add 4 microbenchmarks and set reviewer.
Comment 8 Alexey Shvayka 2019-09-02 15:22:41 PDT
(In reply to Saam Barati from comment #6)
> Comment on attachment 376429 [details]
> Patch
> 
> nice. Can you verify this doesn't make JSON parsing slower?

Please note that changes in Walker::walk may affect only JSON.parse with custom reviver (2nd argument).
I've added two sets of microbenchmarks: reviver returning same or different value.
We will find them useful in https://bugs.webkit.org/show_bug.cgi?id=163446 as well.

Cold runs, --outer 16:

                                            rev. 249368              patch

json-parse-object-reviver-same-value      196.5163+-1.1350      198.7460+-1.4635
json-parse-array-reviver-same-value       143.3628+-1.1241      150.7418+-1.2160
json-parse-object-reviver                 217.9791+-1.2748      223.2554+-1.9196
json-parse-array-reviver                  197.3965+-3.8484      198.1227+-1.9369

<geometric>                               186.5654+-1.0157      190.7782+-0.8042
Comment 9 Alexey Shvayka 2019-10-08 12:32:12 PDT
Created attachment 380451 [details]
Patch

Rebase patch.
Comment 10 WebKit Commit Bot 2019-10-08 14:23:22 PDT
Comment on attachment 380451 [details]
Patch

Clearing flags on attachment: 380451

Committed r250860: <https://trac.webkit.org/changeset/250860>
Comment 11 WebKit Commit Bot 2019-10-08 14:23:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-10-08 14:24:20 PDT
<rdar://problem/56088749>