Skip to content

Commit 31005f3

Browse files
aamCommit Queue
aam
authored and
Commit Queue
committed
[vm/shared] Enforce trivially-shareable values for shared field initial values.
Fix validation rules so typed-data is allowed to be stored in the shared fields. Adjust tsan tests so they can still put user-defined class and list into a shared field. Fixes #61260 TEST=ci Change-Id: If69189c56146d605fc119c8c07f3b42a394912fc Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/444203 Reviewed-by: Ryan Macnak <[email protected]> Commit-Queue: Alexander Aprelev <[email protected]>
1 parent 3bf3383 commit 31005f3

File tree

7 files changed

+140
-18
lines changed

7 files changed

+140
-18
lines changed

‎runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1234,6 +1234,20 @@ intptr_t ReturnIntPtrMethod(Dart_Handle self, intptr_t value) {
12341234
return value;
12351235
}
12361236

1237+
void UnsafeSetSharedTo(Dart_Handle library_name,
1238+
Dart_Handle field_name,
1239+
Dart_Handle value_handle) {
1240+
struct {
1241+
void* library_name_handle;
1242+
void* field_name_handle;
1243+
void* value_handle;
1244+
} args = {.library_name_handle = reinterpret_cast<void*>(library_name),
1245+
.field_name_handle = reinterpret_cast<void*>(field_name),
1246+
.value_handle = reinterpret_cast<void*>(value_handle)};
1247+
1248+
Dart_ExecuteInternalCommand("unsafe-set-shared-to", &args);
1249+
}
1250+
12371251
static void* FfiNativeResolver(const char* name, uintptr_t args_n) {
12381252
if (strcmp(name, "Dart_SetNativeInstanceField") == 0 && args_n == 3) {
12391253
return reinterpret_cast<void*>(Dart_SetNativeInstanceField);
@@ -1289,6 +1303,9 @@ static void* FfiNativeResolver(const char* name, uintptr_t args_n) {
12891303
if (strcmp(name, "ReturnIntPtrMethod") == 0 && args_n == 2) {
12901304
return reinterpret_cast<void*>(ReturnIntPtrMethod);
12911305
}
1306+
if (strcmp(name, "UnsafeSetSharedTo") == 0 && args_n == 3) {
1307+
return reinterpret_cast<void*>(UnsafeSetSharedTo);
1308+
}
12921309
// This should be unreachable in tests.
12931310
ENSURE(false);
12941311
}

‎runtime/tests/vm/dart/tsan/array_data_race_test.dart‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,19 +4,23 @@
44

55
// VMOptions=--experimental-shared-data
66

7+
import "dart:ffi";
78
import "dart:io";
89
import "dart:isolate";
910
import "package:expect/expect.dart";
1011

12+
import '../dylib_utils.dart';
13+
14+
@pragma("vm:entry-point")
1115
@pragma("vm:shared")
12-
List<dynamic> box = List<dynamic>.filled(1, 0);
16+
List<dynamic>? box;
1317

1418
@pragma("vm:never-inline")
1519
noopt() {}
1620

1721
@pragma("vm:never-inline")
1822
dataRaceFromMain() {
19-
final localBox = box;
23+
final localBox = box!;
2024
for (var i = 0; i < 1000000; i++) {
2125
localBox[0] += 1;
2226
noopt();
@@ -25,7 +29,7 @@ dataRaceFromMain() {
2529

2630
@pragma("vm:never-inline")
2731
dataRaceFromChild() {
28-
final localBox = box;
32+
final localBox = box!;
2933
for (var i = 0; i < 1000000; i++) {
3034
localBox[0] += 1;
3135
noopt();
@@ -36,8 +40,26 @@ child(_) {
3640
dataRaceFromChild();
3741
}
3842

43+
final nativeLib = dlopenPlatformSpecific('ffi_test_functions');
44+
45+
final getRootLibraryUrl = nativeLib
46+
.lookupFunction<Handle Function(), Object Function()>('GetRootLibraryUrl');
47+
48+
final setFfiNativeResolverForTest = nativeLib
49+
.lookupFunction<Void Function(Handle), void Function(Object)>(
50+
'SetFfiNativeResolverForTest',
51+
);
52+
53+
@Native<IntPtr Function(Handle, Handle, Handle)>(symbol: 'UnsafeSetSharedTo')
54+
external int unsafeSetSharedTo(Object library_name, String name, Object value);
55+
3956
main(List<String> arguments) {
4057
if (arguments.contains("--testee")) {
58+
setFfiNativeResolverForTest(getRootLibraryUrl());
59+
// At this point List is not allowed to be stored in shaded fields.
60+
// Still we want to use it here to test data race detection.
61+
unsafeSetSharedTo(getRootLibraryUrl(), "box", List<dynamic>.filled(1, 0));
62+
4163
print(box); // side effect initialization
4264
Isolate.spawn(child, null);
4365
dataRaceFromMain();
@@ -61,6 +83,6 @@ main(List<String> arguments) {
6183
Expect.notEquals(0, result.exitCode);
6284
Expect.contains("ThreadSanitizer: data race", result.stderr);
6385
Expect.contains("of size 8", result.stderr);
64-
Expect.contains("dataRaceFromMain", result.stderr);
65-
Expect.contains("dataRaceFromChild", result.stderr);
86+
Expect.contains("List.[]=", result.stderr);
87+
Expect.contains("_Array.[]", result.stderr);
6688
}

‎runtime/tests/vm/dart/tsan/field_data_race_no_sanitize_test.dart‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,24 +4,28 @@
44

55
// VMOptions=--experimental-shared-data
66

7+
import "dart:ffi";
78
import "dart:io";
89
import "dart:isolate";
910
import "package:expect/expect.dart";
1011

12+
import '../dylib_utils.dart';
13+
1114
class Box {
1215
@pragma("vm:no-sanitize-thread") // __attribute__((no_sanitize("thread")))
1316
int foo = 0;
1417
}
1518

19+
@pragma("vm:entry-point")
1620
@pragma("vm:shared")
17-
Box box = Box();
21+
Box? box;
1822

1923
@pragma("vm:never-inline")
2024
noopt() {}
2125

2226
@pragma("vm:never-inline")
2327
dataRaceFromMain() {
24-
final localBox = box;
28+
final localBox = box!;
2529
for (var i = 0; i < 1000000; i++) {
2630
localBox.foo += 1;
2731
noopt();
@@ -30,7 +34,7 @@ dataRaceFromMain() {
3034

3135
@pragma("vm:never-inline")
3236
dataRaceFromChild() {
33-
final localBox = box;
37+
final localBox = box!;
3438
for (var i = 0; i < 1000000; i++) {
3539
localBox.foo += 1;
3640
noopt();
@@ -41,8 +45,23 @@ child(_) {
4145
dataRaceFromChild();
4246
}
4347

48+
final nativeLib = dlopenPlatformSpecific('ffi_test_functions');
49+
50+
final getRootLibraryUrl = nativeLib
51+
.lookupFunction<Handle Function(), Object Function()>('GetRootLibraryUrl');
52+
53+
final setFfiNativeResolverForTest = nativeLib
54+
.lookupFunction<Void Function(Handle), void Function(Object)>(
55+
'SetFfiNativeResolverForTest',
56+
);
57+
58+
@Native<IntPtr Function(Handle, Handle, Handle)>(symbol: 'UnsafeSetSharedTo')
59+
external int unsafeSetSharedTo(Object library_name, String name, Object value);
60+
4461
main(List<String> arguments) {
4562
if (arguments.contains("--testee")) {
63+
setFfiNativeResolverForTest(getRootLibraryUrl());
64+
unsafeSetSharedTo(getRootLibraryUrl(), "box", Box());
4665
print(box); // side effect initialization
4766
Isolate.spawn(child, null);
4867
dataRaceFromMain();

‎runtime/tests/vm/dart/tsan/field_data_race_test.dart‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,23 +4,27 @@
44

55
// VMOptions=--experimental-shared-data
66

7+
import "dart:ffi";
78
import "dart:io";
89
import "dart:isolate";
910
import "package:expect/expect.dart";
1011

12+
import '../dylib_utils.dart';
13+
1114
class Box {
1215
int foo = 0;
1316
}
1417

18+
@pragma("vm:entry-point")
1519
@pragma("vm:shared")
16-
Box box = Box();
20+
Box? box;
1721

1822
@pragma("vm:never-inline")
1923
noopt() {}
2024

2125
@pragma("vm:never-inline")
2226
dataRaceFromMain() {
23-
final localBox = box;
27+
final localBox = box!;
2428
for (var i = 0; i < 1000000; i++) {
2529
localBox.foo += 1;
2630
noopt();
@@ -29,7 +33,7 @@ dataRaceFromMain() {
2933

3034
@pragma("vm:never-inline")
3135
dataRaceFromChild() {
32-
final localBox = box;
36+
final localBox = box!;
3337
for (var i = 0; i < 1000000; i++) {
3438
localBox.foo += 1;
3539
noopt();
@@ -40,8 +44,25 @@ child(_) {
4044
dataRaceFromChild();
4145
}
4246

47+
final nativeLib = dlopenPlatformSpecific('ffi_test_functions');
48+
49+
final getRootLibraryUrl = nativeLib
50+
.lookupFunction<Handle Function(), Object Function()>('GetRootLibraryUrl');
51+
52+
final setFfiNativeResolverForTest = nativeLib
53+
.lookupFunction<Void Function(Handle), void Function(Object)>(
54+
'SetFfiNativeResolverForTest',
55+
);
56+
57+
@Native<IntPtr Function(Handle, Handle, Handle)>(symbol: 'UnsafeSetSharedTo')
58+
external int unsafeSetSharedTo(Object library_name, String name, Object value);
59+
4360
main(List<String> arguments) {
4461
if (arguments.contains("--testee")) {
62+
setFfiNativeResolverForTest(getRootLibraryUrl());
63+
// At this point List is not allowed to be stored in shaded fields.
64+
// Still we want to use it here to test data race detection.
65+
unsafeSetSharedTo(getRootLibraryUrl(), "box", Box());
4566
print(box); // side effect initialization
4667
Isolate.spawn(child, null);
4768
dataRaceFromMain();

‎runtime/vm/ffi_callback_metadata.cc‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -363,14 +363,18 @@ static void ValidateTriviallyImmutabilityOfAnObject(Zone* zone,
363363
zone, Closure::RawCast(p_obj->ptr()));
364364
return;
365365
}
366-
if (!p_obj->IsImmutable()) {
367-
const String& error = String::Handle(
368-
zone,
369-
String::NewFormatted("Only trivially-immutable values are allowed: %s.",
370-
p_obj->ToCString()));
371-
Exceptions::ThrowArgumentError(error);
372-
UNREACHABLE();
366+
if (p_obj->IsImmutable()) {
367+
return;
373368
}
369+
if (IsTypedDataBaseClassId(p_obj->GetClassId())) {
370+
return;
371+
}
372+
const String& error = String::Handle(
373+
zone,
374+
String::NewFormatted("Only trivially-immutable values are allowed: %s.",
375+
p_obj->ToCString()));
376+
Exceptions::ThrowArgumentError(error);
377+
UNREACHABLE();
374378
}
375379

376380
void FfiCallbackMetadata::EnsureOnlyTriviallyImmutableValuesInClosure(

‎runtime/vm/native_api_impl.cc‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,12 @@ struct RunInSafepointAndRWCodeArgs {
284284
std::function<void()>* callback;
285285
};
286286

287+
struct UnsafeSetSharedToArgs {
288+
void* library_name_handle;
289+
void* field_name_handle;
290+
void* value_handle;
291+
};
292+
287293
DART_EXPORT void* Dart_ExecuteInternalCommand(const char* command, void* arg) {
288294
if (strcmp(command, "gc-on-nth-allocation") == 0) {
289295
Thread* const thread = Thread::Current();
@@ -340,6 +346,30 @@ DART_EXPORT void* Dart_ExecuteInternalCommand(const char* command, void* arg) {
340346
Thread::ExitIsolateGroupAsHelper(kBypassSafepoint);
341347
return nullptr;
342348

349+
} else if (strcmp(command, "unsafe-set-shared-to") == 0) {
350+
const UnsafeSetSharedToArgs* const args =
351+
reinterpret_cast<UnsafeSetSharedToArgs*>(arg);
352+
DARTSCOPE(Thread::Current());
353+
const String& library_name = Api::UnwrapStringHandle(
354+
T->zone(), reinterpret_cast<Dart_Handle>(args->library_name_handle));
355+
const String& field_name = Api::UnwrapStringHandle(
356+
T->zone(), reinterpret_cast<Dart_Handle>(args->field_name_handle));
357+
const Instance& value = Api::UnwrapInstanceHandle(
358+
T->zone(), reinterpret_cast<Dart_Handle>(args->value_handle));
359+
const Library& library =
360+
Library::Handle(T->zone(), Library::LookupLibrary(T, library_name));
361+
ASSERT(!library.IsNull());
362+
Field& field =
363+
Field::Handle(T->zone(), library.LookupFieldAllowPrivate(field_name));
364+
ASSERT(!field.IsNull());
365+
if (field.is_shared()) {
366+
SafepointReadRwLocker ml(T, T->isolate_group()->program_lock());
367+
T->isolate_group()->shared_field_table()->SetAt(field.field_id(),
368+
value.ptr());
369+
} else {
370+
field.SetStaticValue(value);
371+
}
372+
return nullptr;
343373
} else {
344374
UNREACHABLE();
345375
}

‎runtime/vm/object.cc‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13400,6 +13400,15 @@ void Field::SetStaticValue(const Object& value) const {
1340013400
const intptr_t id = field_id();
1340113401
ASSERT(id >= 0);
1340213402

13403+
if (is_shared() && !value.IsImmutable() &&
13404+
!IsTypedDataBaseClassId(value.GetClassId())) {
13405+
const String& error = String::Handle(
13406+
thread->zone(),
13407+
String::NewFormatted("Only trivially-immutable values are allowed: %s.",
13408+
value.ToCString()));
13409+
Exceptions::ThrowArgumentError(error);
13410+
UNREACHABLE();
13411+
}
1340313412
SafepointReadRwLocker ml(thread, thread->isolate_group()->program_lock());
1340413413
if (is_shared()) {
1340513414
thread->isolate_group()->shared_field_table()->SetAt(

0 commit comments

Comments
 (0)