Skip to content

Commit bb98b38

Browse files
joyeecheungV8 LUCI CQ
joyeecheung
authored and
V8 LUCI CQ
committed
[ic] handle access check for private names
Previously the LookupIterator ignores private symbols (including private names) for the access check. This patch removes these exceptions so that they are always checked. Drive-by: removes the unused should_throw parameter in Runtime::DefineObjectOwnProperty() Bug: chromium:1321899 Change-Id: I9677b1e377f01d966daa1603eee1ed9535ffab92 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3623419 Reviewed-by: Toon Verwaest <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#80700}
1 parent 78c9466 commit bb98b38

13 files changed

Lines changed: 271 additions & 43 deletions

‎src/ic/ic.cc‎

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1848,19 +1848,10 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
18481848
IsAnyDefineOwn() ? LookupIterator::OWN : LookupIterator::DEFAULT);
18491849

18501850
if (name->IsPrivate()) {
1851-
bool exists = it.IsFound();
1852-
if (name->IsPrivateName() && exists == IsDefineKeyedOwnIC()) {
1853-
Handle<String> name_string(
1854-
String::cast(Symbol::cast(*name).description()), isolate());
1855-
if (exists) {
1856-
MessageTemplate message =
1857-
name->IsPrivateBrand()
1858-
? MessageTemplate::kInvalidPrivateBrandReinitialization
1859-
: MessageTemplate::kInvalidPrivateFieldReinitialization;
1860-
return TypeError(message, object, name_string);
1861-
} else {
1862-
return TypeError(MessageTemplate::kInvalidPrivateMemberWrite, object,
1863-
name_string);
1851+
if (name->IsPrivateName()) {
1852+
DCHECK(!IsDefineNamedOwnIC());
1853+
if (!JSReceiver::CheckPrivateNameStore(&it, IsDefineKeyedOwnIC())) {
1854+
return MaybeHandle<Object>();
18641855
}
18651856
}
18661857

‎src/objects/js-objects.cc‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,55 @@ Maybe<bool> JSReceiver::HasInPrototypeChain(Isolate* isolate,
185185
}
186186
}
187187

188+
// static
189+
bool JSReceiver::CheckPrivateNameStore(LookupIterator* it, bool is_define) {
190+
DCHECK(it->GetName()->IsPrivateName());
191+
Isolate* isolate = it->isolate();
192+
Handle<String> name_string(
193+
String::cast(Handle<Symbol>::cast(it->GetName())->description()),
194+
isolate);
195+
bool should_throw = GetShouldThrow(isolate, Nothing<ShouldThrow>()) ==
196+
ShouldThrow::kThrowOnError;
197+
for (; it->IsFound(); it->Next()) {
198+
switch (it->state()) {
199+
case LookupIterator::TRANSITION:
200+
case LookupIterator::INTERCEPTOR:
201+
case LookupIterator::JSPROXY:
202+
case LookupIterator::NOT_FOUND:
203+
case LookupIterator::INTEGER_INDEXED_EXOTIC:
204+
case LookupIterator::ACCESSOR:
205+
UNREACHABLE();
206+
case LookupIterator::ACCESS_CHECK:
207+
if (!it->HasAccess()) {
208+
isolate->ReportFailedAccessCheck(
209+
Handle<JSObject>::cast(it->GetReceiver()));
210+
RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, false);
211+
return false;
212+
}
213+
break;
214+
case LookupIterator::DATA:
215+
if (is_define && should_throw) {
216+
MessageTemplate message =
217+
it->GetName()->IsPrivateBrand()
218+
? MessageTemplate::kInvalidPrivateBrandReinitialization
219+
: MessageTemplate::kInvalidPrivateFieldReinitialization;
220+
isolate->Throw(*(isolate->factory()->NewTypeError(
221+
message, name_string, it->GetReceiver())));
222+
return false;
223+
}
224+
return true;
225+
}
226+
}
227+
DCHECK(!it->IsFound());
228+
if (!is_define && should_throw) {
229+
isolate->Throw(*(isolate->factory()->NewTypeError(
230+
MessageTemplate::kInvalidPrivateMemberWrite, name_string,
231+
it->GetReceiver())));
232+
return false;
233+
}
234+
return true;
235+
}
236+
188237
// static
189238
Maybe<bool> JSReceiver::CheckIfCanDefine(Isolate* isolate, LookupIterator* it,
190239
Handle<Object> value,

‎src/objects/js-objects.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,11 @@ class JSReceiver : public TorqueGeneratedJSReceiver<JSReceiver, HeapObject> {
161161
Isolate* isolate, Handle<JSReceiver> object, Handle<Object> key,
162162
PropertyDescriptor* desc, Maybe<ShouldThrow> should_throw);
163163

164+
// Check if private name property can be store on the object. It will return
165+
// false with an error when it cannot.
166+
V8_WARN_UNUSED_RESULT static bool CheckPrivateNameStore(LookupIterator* it,
167+
bool is_define);
168+
164169
// Check if a data property can be created on the object. It will fail with
165170
// an error when it cannot.
166171
V8_WARN_UNUSED_RESULT static Maybe<bool> CheckIfCanDefine(

‎src/objects/lookup.cc‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1265,7 +1265,9 @@ LookupIterator::State LookupIterator::LookupInSpecialHolder(
12651265
}
12661266
#endif // V8_ENABLE_WEBASSEMBLY
12671267
if (map.is_access_check_needed()) {
1268-
if (is_element || !name_->IsPrivate(isolate_)) return ACCESS_CHECK;
1268+
if (is_element || !name_->IsPrivate(isolate_) ||
1269+
name_->IsPrivateName(isolate_))
1270+
return ACCESS_CHECK;
12691271
}
12701272
V8_FALLTHROUGH;
12711273
case ACCESS_CHECK:

‎src/runtime/runtime-object.cc‎

Lines changed: 18 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ MaybeHandle<Object> Runtime::GetObjectProperty(
4848
LookupIterator(isolate, receiver, lookup_key, lookup_start_object);
4949

5050
MaybeHandle<Object> result = Object::GetProperty(&it);
51+
if (result.is_null()) {
52+
return result;
53+
}
5154
if (is_found) *is_found = it.IsFound();
5255

5356
if (!it.IsFound() && key->IsSymbol() &&
@@ -564,15 +567,9 @@ MaybeHandle<Object> Runtime::SetObjectProperty(
564567
PropertyKey lookup_key(isolate, key, &success);
565568
if (!success) return MaybeHandle<Object>();
566569
LookupIterator it(isolate, object, lookup_key);
567-
568-
if (!it.IsFound() && key->IsSymbol() &&
569-
Symbol::cast(*key).is_private_name()) {
570-
Handle<Object> name_string(Symbol::cast(*key).description(), isolate);
571-
DCHECK(name_string->IsString());
572-
THROW_NEW_ERROR(isolate,
573-
NewTypeError(MessageTemplate::kInvalidPrivateMemberWrite,
574-
name_string, object),
575-
Object);
570+
if (key->IsSymbol() && Symbol::cast(*key).is_private_name() &&
571+
!JSReceiver::CheckPrivateNameStore(&it, false)) {
572+
return MaybeHandle<Object>();
576573
}
577574

578575
MAYBE_RETURN_NULL(
@@ -581,10 +578,11 @@ MaybeHandle<Object> Runtime::SetObjectProperty(
581578
return value;
582579
}
583580

584-
MaybeHandle<Object> Runtime::DefineObjectOwnProperty(
585-
Isolate* isolate, Handle<Object> object, Handle<Object> key,
586-
Handle<Object> value, StoreOrigin store_origin,
587-
Maybe<ShouldThrow> should_throw) {
581+
MaybeHandle<Object> Runtime::DefineObjectOwnProperty(Isolate* isolate,
582+
Handle<Object> object,
583+
Handle<Object> key,
584+
Handle<Object> value,
585+
StoreOrigin store_origin) {
588586
if (object->IsNullOrUndefined(isolate)) {
589587
THROW_NEW_ERROR(
590588
isolate,
@@ -599,20 +597,15 @@ MaybeHandle<Object> Runtime::DefineObjectOwnProperty(
599597
LookupIterator it(isolate, object, lookup_key, LookupIterator::OWN);
600598

601599
if (key->IsSymbol() && Symbol::cast(*key).is_private_name()) {
602-
Handle<Symbol> private_symbol = Handle<Symbol>::cast(key);
603-
if (it.IsFound()) {
604-
Handle<Object> name_string(private_symbol->description(), isolate);
605-
DCHECK(name_string->IsString());
606-
MessageTemplate message =
607-
private_symbol->is_private_brand()
608-
? MessageTemplate::kInvalidPrivateBrandReinitialization
609-
: MessageTemplate::kInvalidPrivateFieldReinitialization;
610-
THROW_NEW_ERROR(isolate, NewTypeError(message, name_string), Object);
611-
} else {
612-
MAYBE_RETURN_NULL(JSReceiver::AddPrivateField(&it, value, should_throw));
600+
if (!JSReceiver::CheckPrivateNameStore(&it, true)) {
601+
return MaybeHandle<Object>();
613602
}
603+
DCHECK(!it.IsFound());
604+
MAYBE_RETURN_NULL(
605+
JSReceiver::AddPrivateField(&it, value, Nothing<ShouldThrow>()));
614606
} else {
615-
MAYBE_RETURN_NULL(JSReceiver::CreateDataProperty(&it, value, should_throw));
607+
MAYBE_RETURN_NULL(
608+
JSReceiver::CreateDataProperty(&it, value, Nothing<ShouldThrow>()));
616609
}
617610

618611
return value;

‎src/runtime/runtime.h‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -826,10 +826,9 @@ class Runtime : public AllStatic {
826826
// private field definition), this method throws if the field already exists
827827
// on object.
828828
V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static MaybeHandle<Object>
829-
DefineObjectOwnProperty(
830-
Isolate* isolate, Handle<Object> object, Handle<Object> key,
831-
Handle<Object> value, StoreOrigin store_origin,
832-
Maybe<ShouldThrow> should_throw = Nothing<ShouldThrow>());
829+
DefineObjectOwnProperty(Isolate* isolate, Handle<Object> object,
830+
Handle<Object> key, Handle<Object> value,
831+
StoreOrigin store_origin);
833832

834833
// When "receiver" is not passed, it defaults to "lookup_start_object".
835834
V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static MaybeHandle<Object>

‎test/mjsunit/regress/regress-crbug-1321899-1.js‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// Copyright 2022 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
d8.file.execute('test/mjsunit/regress/regress-crbug-1321899.js');
6+
7+
// Detached global should not have access
8+
const realm = Realm.createAllowCrossRealmAccess();
9+
const detached = Realm.global(realm);
10+
Realm.detachGlobal(realm);
11+
12+
assertThrows(() => new B(detached), Error, /no access/);
13+
assertThrows(() => new C(detached), Error, /no access/);
14+
assertThrows(() => new D(detached), Error, /no access/);
15+
assertThrows(() => new E(detached), Error, /no access/);
16+
assertThrows(() => B.setField(detached), Error, /no access/);
17+
assertThrows(() => C.setField(detached), Error, /no access/);
18+
assertThrows(() => D.setAccessor(detached), Error, /no access/);
19+
assertThrows(() => E.setMethod(detached), Error, /no access/);
20+
assertThrows(() => D.getAccessor(detached), Error, /no access/);
21+
assertThrows(() => E.getMethod(detached), Error, /no access/);

‎test/mjsunit/regress/regress-crbug-1321899-2.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
// Copyright 2022 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --no-lazy-feedback-allocation
6+
7+
d8.file.execute('test/mjsunit/regress/regress-crbug-1321899-1.js');

‎test/mjsunit/regress/regress-crbug-1321899-3.js‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
// Copyright 2022 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
d8.file.execute('test/mjsunit/regress/regress-crbug-1321899.js');
6+
7+
// Attached global should have access
8+
const realm = Realm.createAllowCrossRealmAccess();
9+
const globalProxy = Realm.global(realm);
10+
11+
assertThrows(() => B.setField(globalProxy), TypeError, /Cannot write private member #b to an object whose class did not declare it/);
12+
assertThrows(() => B.getField(globalProxy), TypeError, /Cannot read private member #b from an object whose class did not declare it/);
13+
14+
new B(globalProxy);
15+
assertEquals(B.getField(globalProxy), 1);
16+
B.setField(globalProxy);
17+
assertEquals(B.getField(globalProxy), 'b'); // Fast case
18+
B.setField(globalProxy); // Fast case
19+
assertEquals(B.getField(globalProxy), 'b'); // Fast case
20+
assertThrows(() => new B(globalProxy), TypeError, /Cannot initialize #b twice on the same object/);
21+
22+
assertThrows(() => C.setField(globalProxy), TypeError, /Cannot write private member #c to an object whose class did not declare it/);
23+
assertThrows(() => C.getField(globalProxy), TypeError, /Cannot read private member #c from an object whose class did not declare it/);
24+
25+
new C(globalProxy);
26+
assertEquals(C.getField(globalProxy), undefined);
27+
C.setField(globalProxy);
28+
assertEquals(C.getField(globalProxy), 'c'); // Fast case
29+
C.setField(globalProxy); // Fast case
30+
assertEquals(C.getField(globalProxy), 'c'); // Fast case
31+
assertThrows(() => new C(globalProxy), TypeError, /Cannot initialize #c twice on the same object/);
32+
33+
assertThrows(() => D.setAccessor(globalProxy), TypeError, /Receiver must be an instance of class D/);
34+
assertThrows(() => D.getAccessor(globalProxy), TypeError, /Receiver must be an instance of class D/);
35+
36+
new D(globalProxy);
37+
assertEquals(D.getAccessor(globalProxy), 0);
38+
D.setAccessor(globalProxy);
39+
assertEquals(D.getAccessor(globalProxy), 'd'); // Fast case
40+
D.setAccessor(globalProxy); // Fast case
41+
assertEquals(D.getAccessor(globalProxy), 'd'); // Fast case
42+
assertThrows(() => new D(globalProxy), TypeError, /Cannot initialize private methods of class D twice on the same object/);
43+
44+
assertThrows(() => E.setMethod(globalProxy), TypeError, /Receiver must be an instance of class E/);
45+
assertThrows(() => E.getMethod(globalProxy), TypeError, /Receiver must be an instance of class E/);
46+
47+
new E(globalProxy);
48+
assertEquals(E.getMethod(globalProxy)(), 0);
49+
assertThrows(() => E.setMethod(globalProxy), TypeError, /Private method '#e' is not writable/);
50+
assertEquals(E.getMethod(globalProxy)(), 0); // Fast case
51+
assertThrows(() => new E(globalProxy), TypeError, /Cannot initialize private methods of class E twice on the same object/);
52+
53+
// Access should fail after detaching
54+
Realm.detachGlobal(realm);
55+
56+
assertThrows(() => new B(globalProxy), Error, /no access/);
57+
assertThrows(() => new C(globalProxy), Error, /no access/);
58+
assertThrows(() => new D(globalProxy), Error, /no access/);
59+
assertThrows(() => new E(globalProxy), Error, /no access/);
60+
assertThrows(() => B.setField(globalProxy), Error, /no access/);
61+
assertThrows(() => C.setField(globalProxy), Error, /no access/);
62+
assertThrows(() => D.setAccessor(globalProxy), Error, /no access/);
63+
assertThrows(() => E.setMethod(globalProxy), Error, /no access/);
64+
assertThrows(() => D.getAccessor(globalProxy), Error, /no access/);
65+
assertThrows(() => E.getMethod(globalProxy), Error, /no access/);

‎test/mjsunit/regress/regress-crbug-1321899-4.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
// Copyright 2022 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --no-lazy-feedback-allocation
6+
7+
d8.file.execute('test/mjsunit/regress/regress-crbug-1321899-3.js');

0 commit comments

Comments
 (0)