Bug 163430 - JSON.parse should not modify frozen objects
Summary: JSON.parse should not modify frozen objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
: 122759 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-10-13 23:43 PDT by Mark Lam
Modified: 2018-10-04 12:16 PDT (History)
8 users (show)

See Also:


Attachments
proposed patch. (8.95 KB, patch)
2016-10-13 23:54 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-10-13 23:43:33 PDT
The ES6 spec for JSON.parse (https://tc39.github.io/ecma262/#sec-json.parse and https://tc39.github.io/ecma262/#sec-internalizejsonproperty) states that it uses CreateDataProperty() (https://tc39.github.io/ecma262/#sec-createdataproperty) to set values returned by a reviver.  The spec for CreateDataPropertyOrThrow states:

"This abstract operation creates a property whose attributes are set to the same defaults used for properties created by the ECMAScript language assignment operator. Normally, the property will not already exist. If it does exist and is not configurable or if O is not extensible, [[DefineOwnProperty]] will return false."

Since the properties of frozen objects are not extensible, not configurable, and not writeable, JSON.parse should fail to write to any frozen objects.  Similarly, JSON.parse should fail to delete properties in frozen objects.
Comment 1 Mark Lam 2016-10-13 23:54:45 PDT
Created attachment 291578 [details]
proposed patch.
Comment 2 Saam Barati 2016-10-14 08:04:44 PDT
Comment on attachment 291578 [details]
proposed patch.

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

> JSTests/stress/json-parse-on-frozen-object.js:11
> +        this.b = Object.freeze(["unmodifiable"]);

This is a nice test file. 
Can you also add a test for making the property at 0 non-configurable and non-writable?

> JSTests/stress/json-parse-on-frozen-object.js:38
> +        return undefined;

The behavior here is to indicate that property 0 should be deleted?
Comment 3 Mark Lam 2016-10-14 08:32:14 PDT
(In reply to comment #2)
> Comment on attachment 291578 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291578&action=review
> 
> > JSTests/stress/json-parse-on-frozen-object.js:11
> > +        this.b = Object.freeze(["unmodifiable"]);
> 
> This is a nice test file. 
> Can you also add a test for making the property at 0 non-configurable and
> non-writable?

I'll work on that.
 
> > JSTests/stress/json-parse-on-frozen-object.js:38
> > +        return undefined;
> 
> The behavior here is to indicate that property 0 should be deleted?

Yes.  It's in the spec.  If the reviver returns undefined, JSON.parse deletes the property.
Comment 4 Mark Lam 2016-10-14 08:58:59 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Can you also add a test for making the property at 0 non-configurable and
> > non-writable?
> 
> I'll work on that.

Interesting.  This uncovered a bug in a different area. I'll land this patch first, and fix that in a separate bug and add the non-configurable non-writable test there.
Comment 5 Mark Lam 2016-10-14 09:06:01 PDT
I'll address the bug with non-configurable properties in https://bugs.webkit.org/show_bug.cgi?id=163446.

Landed in r207341: <http://trac.webkit.org/r207341>.
Comment 6 Mark Lam 2018-10-04 12:14:44 PDT
<rdar://problem/15221406>
Comment 7 Mark Lam 2018-10-04 12:16:22 PDT
*** Bug 122759 has been marked as a duplicate of this bug. ***