Skip to content

Commit 79b967c

Browse files
shvaikaleshwebkit-commit-queue
shvaikalesh
authored and
webkit-commit-queue
committed
Three checks are missing in Proxy internal methods
https://bugs.webkit.org/show_bug.cgi?id=198630 Patch by Alexey Shvayka <[email protected]> on 2019-07-24 Reviewed by Darin Adler. JSTests: * stress/proxy-delete.js: Assert isExtensible is called in correct order. * test262/expectations.yaml: Mark 6 test cases as passing. Source/JavaScriptCore: Add three missing checks in Proxy internal methods. These checks are necessary to maintain the invariants of the essential internal methods. (tc39/ecma262#666) 1. [[GetOwnProperty]] shouldn't return non-configurable and non-writable descriptor when the target's property is writable. 2. [[Delete]] should return `false` when the target has property and is not extensible. 3. [[DefineOwnProperty]] should return `true` for a non-writable input descriptor when the target's property is non-configurable and writable. Shipping in SpiderMonkey since https://hg.mozilla.org/integration/autoland/rev/3a06bc818bc4 (version 69) Shipping in V8 since https://chromium.googlesource.com/v8/v8.git/+/e846ad9fa5109428be50b1989314e0e4e7267919 * runtime/ProxyObject.cpp: (JSC::ProxyObject::performInternalMethodGetOwnProperty): Add writability check. (JSC::ProxyObject::performDelete): Add extensibility check. (JSC::ProxyObject::performDefineOwnProperty): Add writability check. Canonical link: https://commits.webkit.org/213935@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247811 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 2a73c17 commit 79b967c

File tree

5 files changed

+90
-9
lines changed

5 files changed

+90
-9
lines changed

‎JSTests/ChangeLog‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,13 @@
1+
2019-07-24 Alexey Shvayka <[email protected]>
2+
3+
Three checks are missing in Proxy internal methods
4+
https://bugs.webkit.org/show_bug.cgi?id=198630
5+
6+
Reviewed by Darin Adler.
7+
8+
* stress/proxy-delete.js: Assert isExtensible is called in correct order.
9+
* test262/expectations.yaml: Mark 6 test cases as passing.
10+
111
2019-07-23 Justin Michaud <[email protected]>
212

313
Sometimes we miss removable CheckInBounds

‎JSTests/stress/proxy-delete.js‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,47 @@ function assert(b) {
9292
}
9393
}
9494

95+
{
96+
let target = new Proxy({}, {
97+
isExtensible: function() {
98+
throw new Error("should not be called if [[GetOwnProperty]] returns undefined");
99+
}
100+
});
101+
let proxy = new Proxy(target, {
102+
deleteProperty: function() {
103+
return true;
104+
}
105+
});
106+
for (let i = 0; i < 500; i++) {
107+
assert(delete proxy.nonExistentProperty);
108+
}
109+
}
110+
111+
{
112+
let calls;
113+
let target = new Proxy({}, {
114+
getOwnPropertyDescriptor: function() {
115+
calls.push('getOwnPropertyDescriptor');
116+
return { configurable: true };
117+
},
118+
isExtensible: function() {
119+
calls.push('isExtensible');
120+
return true;
121+
}
122+
});
123+
let proxy = new Proxy(target, {
124+
deleteProperty: function() {
125+
calls.push('trap');
126+
return true;
127+
}
128+
});
129+
for (let i = 0; i < 500; i++) {
130+
calls = [];
131+
assert(delete proxy.prop);
132+
assert(calls.join() == 'trap,getOwnPropertyDescriptor,isExtensible');
133+
}
134+
}
135+
95136
{
96137
let target = {};
97138
let error = null;

‎JSTests/test262/expectations.yaml‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1345,15 +1345,6 @@ test/built-ins/Proxy/construct/trap-is-not-callable-realm.js:
13451345
test/built-ins/Proxy/create-handler-is-revoked-proxy.js:
13461346
default: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
13471347
strict mode: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
1348-
test/built-ins/Proxy/defineProperty/targetdesc-not-configurable-writable-desc-not-writable.js:
1349-
default: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
1350-
strict mode: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
1351-
test/built-ins/Proxy/deleteProperty/targetdesc-is-configurable-target-is-not-extensible.js:
1352-
default: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
1353-
strict mode: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
1354-
test/built-ins/Proxy/getOwnPropertyDescriptor/resultdesc-is-not-configurable-not-writable-targetdesc-is-writable.js:
1355-
default: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
1356-
strict mode: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
13571348
test/built-ins/RegExp/named-groups/groups-object-subclass-sans.js:
13581349
default: 'Test262Error: Expected SameValue(«b», «$<a>») to be true'
13591350
strict mode: 'Test262Error: Expected SameValue(«b», «$<a>») to be true'

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,26 @@
1+
2019-07-24 Alexey Shvayka <[email protected]>
2+
3+
Three checks are missing in Proxy internal methods
4+
https://bugs.webkit.org/show_bug.cgi?id=198630
5+
6+
Reviewed by Darin Adler.
7+
8+
Add three missing checks in Proxy internal methods.
9+
These checks are necessary to maintain the invariants of the essential internal methods.
10+
(https://github.com/tc39/ecma262/pull/666)
11+
12+
1. [[GetOwnProperty]] shouldn't return non-configurable and non-writable descriptor when the target's property is writable.
13+
2. [[Delete]] should return `false` when the target has property and is not extensible.
14+
3. [[DefineOwnProperty]] should return `true` for a non-writable input descriptor when the target's property is non-configurable and writable.
15+
16+
Shipping in SpiderMonkey since https://hg.mozilla.org/integration/autoland/rev/3a06bc818bc4 (version 69)
17+
Shipping in V8 since https://chromium.googlesource.com/v8/v8.git/+/e846ad9fa5109428be50b1989314e0e4e7267919
18+
19+
* runtime/ProxyObject.cpp:
20+
(JSC::ProxyObject::performInternalMethodGetOwnProperty): Add writability check.
21+
(JSC::ProxyObject::performDelete): Add extensibility check.
22+
(JSC::ProxyObject::performDefineOwnProperty): Add writability check.
23+
124
2019-07-24 Mark Lam <[email protected]>
225

326
Remove some unused code.

‎Source/JavaScriptCore/runtime/ProxyObject.cpp‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,10 @@ bool ProxyObject::performInternalMethodGetOwnProperty(ExecState* exec, PropertyN
287287
throwVMTypeError(exec, scope, "Result from 'getOwnPropertyDescriptor' can't be non-configurable when the 'target' doesn't have it as an own property or if it is a configurable own property on 'target'"_s);
288288
return false;
289289
}
290+
if (trapResultAsDescriptor.writablePresent() && !trapResultAsDescriptor.writable() && targetPropertyDescriptor.writable()) {
291+
throwVMTypeError(exec, scope, "Result from 'getOwnPropertyDescriptor' can't be non-configurable and non-writable when the target's property is writable"_s);
292+
return false;
293+
}
290294
}
291295

292296
if (trapResultAsDescriptor.isAccessorDescriptor()) {
@@ -662,6 +666,12 @@ bool ProxyObject::performDelete(ExecState* exec, PropertyName propertyName, Defa
662666
throwVMTypeError(exec, scope, "Proxy handler's 'deleteProperty' method should return false when the target's property is not configurable"_s);
663667
return false;
664668
}
669+
bool targetIsExtensible = target->isExtensible(exec);
670+
RETURN_IF_EXCEPTION(scope, false);
671+
if (!targetIsExtensible) {
672+
throwVMTypeError(exec, scope, "Proxy handler's 'deleteProperty' method should return false when the target has property and is not extensible"_s);
673+
return false;
674+
}
665675
}
666676

667677
RETURN_IF_EXCEPTION(scope, false);
@@ -886,6 +896,12 @@ bool ProxyObject::performDefineOwnProperty(ExecState* exec, PropertyName propert
886896
throwVMTypeError(exec, scope, "Proxy's 'defineProperty' trap did not define a non-configurable property on its target even though the input descriptor to the trap said it must do so"_s);
887897
return false;
888898
}
899+
if (targetDescriptor.isDataDescriptor() && !targetDescriptor.configurable() && targetDescriptor.writable()) {
900+
if (descriptor.writablePresent() && !descriptor.writable()) {
901+
throwTypeError(exec, scope, "Proxy's 'defineProperty' trap returned true for a non-writable input descriptor when the target's property is non-configurable and writable"_s);
902+
return false;
903+
}
904+
}
889905

890906
return true;
891907
}

0 commit comments

Comments
 (0)