[vm/shared] Ensure context captured by isolategroup callbacks has only trivially shareable objects.

Fixes #61210
TEST=run_isolate_group_run_test, isolate_group_bound_callback_test

Change-Id: I9633726055007255f67eb9744186fdbe21fa832d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/443639
Commit-Queue: Alexander Aprelev <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>

Author: aam

runtime/lib/concurrent.cc

@@ -4,6 +4,8 @@
 
 #include "include/dart_api.h"
 #include "vm/bootstrap_natives.h"
+#include "vm/dart_api_impl.h"
+#include "vm/ffi_callback_metadata.h"
 #include "vm/heap/safepoint.h"
 #include "vm/os_thread.h"
 
@@ -120,9 +122,15 @@ DEFINE_FFI_NATIVE_ENTRY(IsolateGroup_runSync,
         "Encountered shared data api when functionality is disabled. "
         "Pass --experimental-shared-data");
   }
-
   Thread* current_thread = Thread::Current();
   ASSERT(current_thread->execution_state() == Thread::kThreadInNative);
+
+  {
+    DARTSCOPE(current_thread);
+    FfiCallbackMetadata::EnsureOnlyTriviallyImmutableValuesInClosure(
+        current_thread->zone(), Closure::RawCast(Api::UnwrapHandle(closure)));
+  }
+
   Isolate* saved_isolate = current_thread->isolate();
   current_thread->ExitSafepointFromNative();
   current_thread->set_execution_state(Thread::kThreadInVM);

runtime/vm/ffi_callback_metadata.cc

@@ -351,6 +351,52 @@ PersistentHandle* FfiCallbackMetadata::CreatePersistentHandle(
   return handle;
 }
 
+static void ValidateTriviallyImmutabilityOfAnObject(Zone* zone,
+                                                    Object* p_obj,
+                                                    ObjectPtr object_ptr) {
+  *p_obj = object_ptr;
+  if (p_obj->IsSmi() || p_obj->IsNull()) {
+    return;
+  }
+  if (p_obj->IsClosure()) {
+    FfiCallbackMetadata::EnsureOnlyTriviallyImmutableValuesInClosure(
+        zone, Closure::RawCast(p_obj->ptr()));
+    return;
+  }
+  if (!p_obj->IsImmutable()) {
+    const String& error = String::Handle(
+        zone,
+        String::NewFormatted("Only trivially-immutable values are allowed: %s.",
+                             p_obj->ToCString()));
+    Exceptions::ThrowArgumentError(error);
+    UNREACHABLE();
+  }
+}
+
+void FfiCallbackMetadata::EnsureOnlyTriviallyImmutableValuesInClosure(
+    Zone* zone,
+    ClosurePtr closure_ptr) {
+  Closure& closure = Closure::Handle(zone, closure_ptr);
+  if (closure.IsNull()) {
+    return;
+  }
+  Object& obj = Object::Handle(zone);
+  const auto& function = Function::Handle(closure.function());
+  if (function.IsImplicitClosureFunction()) {
+    ValidateTriviallyImmutabilityOfAnObject(
+        zone, &obj, closure.GetImplicitClosureReceiver());
+  } else {
+    const Context& context = Context::Handle(zone, closure.GetContext());
+    if (context.IsNull()) {
+      return;
+    }
+    // Iterate through all elements of the context.
+    for (intptr_t i = 0; i < context.num_variables(); i++) {
+      ValidateTriviallyImmutabilityOfAnObject(zone, &obj, context.At(i));
+    }
+  }
+}
+
 FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateLocalFfiCallback(
     Isolate* isolate,
     IsolateGroup* isolate_group,
@@ -375,6 +421,12 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateLocalFfiCallback(
            (isolate == nullptr && isolate_group != nullptr &&
             function.GetFfiCallbackKind() ==
                 FfiCallbackKind::kIsolateGroupBoundClosureCallback));
+
+    if (function.GetFfiCallbackKind() ==
+        FfiCallbackKind::kIsolateGroupBoundClosureCallback) {
+      EnsureOnlyTriviallyImmutableValuesInClosure(zone, closure.ptr());
+    }
+
     handle = CreatePersistentHandle(
         isolate != nullptr ? isolate->group() : isolate_group, closure);
   }

runtime/vm/ffi_callback_metadata.h

@@ -346,6 +346,10 @@ class FfiCallbackMetadata {
 #error What architecture?
 #endif
 
+  static void EnsureOnlyTriviallyImmutableValuesInClosure(
+      Zone* zone,
+      ClosurePtr closure_ptr);
+
   // Visible for testing.
   MetadataEntry* MetadataEntryOfTrampoline(Trampoline trampoline) const;
   Trampoline TrampolineOfMetadataEntry(MetadataEntry* metadata) const;

tests/ffi/isolate_group_bound_callback_test.dart

@@ -50,29 +50,24 @@ DynamicLibrary dlopenPlatformSpecific(String name) {
   return DynamicLibrary.open('$_dylibPrefix$name$_dylibExtension');
 }
 
-class NativeLibrary {
-  late final FnRunnerType callFunctionOnSameThread;
-  late final FnRunnerType callFunctionOnNewThreadBlocking;
-  late final FnRunnerType callFunctionOnNewThreadNonBlocking;
-  late final TwoIntFnType callTwoIntFunction;
-  late final FnSleepType sleep;
-
-  NativeLibrary(DynamicLibrary ffiTestFunctions) {
-    callFunctionOnNewThreadNonBlocking = ffiTestFunctions
-        .lookupFunction<FnRunnerNativeType, FnRunnerType>(
-          "CallFunctionOnNewThreadNonBlocking",
-        );
-    callFunctionOnNewThreadBlocking = ffiTestFunctions
-        .lookupFunction<FnRunnerNativeType, FnRunnerType>(
-          "CallFunctionOnNewThreadBlocking",
-        );
-    callTwoIntFunction = ffiTestFunctions
-        .lookupFunction<TwoIntFnNativeType, TwoIntFnType>("CallTwoIntFunction");
-    sleep = ffiTestFunctions.lookupFunction<FnSleepNativeType, FnSleepType>(
-      "SleepFor",
+DynamicLibrary get ffiTestFunctions =>
+    dlopenPlatformSpecific("ffi_test_functions");
+
+FnRunnerType get callFunctionOnNewThreadNonBlocking =>
+    ffiTestFunctions.lookupFunction<FnRunnerNativeType, FnRunnerType>(
+      "CallFunctionOnNewThreadNonBlocking",
     );
-  }
-}
+
+FnRunnerType get callFunctionOnNewThreadBlocking =>
+    ffiTestFunctions.lookupFunction<FnRunnerNativeType, FnRunnerType>(
+      "CallFunctionOnNewThreadBlocking",
+    );
+
+TwoIntFnType get callTwoIntFunction => ffiTestFunctions
+    .lookupFunction<TwoIntFnNativeType, TwoIntFnType>("CallTwoIntFunction");
+
+FnSleepType get sleep =>
+    ffiTestFunctions.lookupFunction<FnSleepNativeType, FnSleepType>("SleepFor");
 
 @pragma('vm:shared')
 late Mutex mutexCondvar;
@@ -88,16 +83,14 @@ const int sleepForMs = 1000;
 
 void simpleFunction(int a, int b) {
   result += (a * b);
-  final ffiTestFunctions = dlopenPlatformSpecific("ffi_test_functions");
-  final lib = NativeLibrary(ffiTestFunctions);
-  lib.sleep(sleepForMs);
+  sleep(sleepForMs);
   mutexCondvar.runLocked(() {
     resultIsReady = true;
     conditionVariable.notify();
   });
 }
 
-Future<void> testNativeCallableHelloWorld(NativeLibrary lib) async {
+Future<void> testNativeCallableHelloWorld() async {
   mutexCondvar = Mutex();
   conditionVariable = ConditionVariable();
   final callback = NativeCallable<CallbackNativeType>.isolateGroupBound(
@@ -106,7 +99,7 @@ Future<void> testNativeCallableHelloWorld(NativeLibrary lib) async {
 
   result = 42;
   resultIsReady = false;
-  lib.callFunctionOnNewThreadNonBlocking(1001, callback.nativeFunction);
+  callFunctionOnNewThreadNonBlocking(1001, callback.nativeFunction);
 
   mutexCondvar.runLocked(() {
     while (!resultIsReady) {
@@ -118,7 +111,7 @@ Future<void> testNativeCallableHelloWorld(NativeLibrary lib) async {
   Expect.equals(42 + (1001 * 123), result);
 
   resultIsReady = false;
-  lib.callFunctionOnNewThreadNonBlocking(1001, callback.nativeFunction);
+  callFunctionOnNewThreadNonBlocking(1001, callback.nativeFunction);
   mutexCondvar.runLocked(() {
     while (!resultIsReady) {
       conditionVariable.wait(mutexCondvar, 10 * sleepForMs);
@@ -134,7 +127,7 @@ void simpleFunctionThatThrows(int a, int b) {
   throw 'hello, world';
 }
 
-Future<void> testNativeCallableThrows(NativeLibrary lib) async {
+Future<void> testNativeCallableThrows() async {
   mutexCondvar = Mutex();
   conditionVariable = ConditionVariable();
   final callback = NativeCallable<CallbackNativeType>.isolateGroupBound(
@@ -147,7 +140,7 @@ Future<void> testNativeCallableThrows(NativeLibrary lib) async {
   // race between invoking the callback and closing it few lines down below.
   // So the main thing this test checks is condition variable timeout,
   // which is still valuable.
-  lib.callFunctionOnNewThreadBlocking(1001, callback.nativeFunction);
+  callFunctionOnNewThreadBlocking(1001, callback.nativeFunction);
 
   mutexCondvar.runLocked(() {
     // Just have short one second sleep - the condition variable is not
@@ -158,15 +151,32 @@ Future<void> testNativeCallableThrows(NativeLibrary lib) async {
   callback.close();
 }
 
-Future<void> testNativeCallableHelloWorldClosure(NativeLibrary lib) async {
+@pragma('vm:shared')
+SendPort? sp;
+
+Future<void> testFailToCaptureReceivePort() async {
+  final rp = ReceivePort();
+  Expect.throws(
+    () {
+      NativeCallable<CallbackNativeType>.isolateGroupBound((int a, int b) {
+        sp = rp.sendPort;
+      });
+    },
+    (e) =>
+        e is ArgumentError && e.toString().contains('Only trivially-immutable'),
+  );
+  rp.close();
+}
+
+Future<void> testNativeCallableHelloWorldClosure() async {
   mutexCondvar = Mutex();
   conditionVariable = ConditionVariable();
   final callback = NativeCallable<CallbackNativeType>.isolateGroupBound((
     int a,
     int b,
   ) {
     result += (a * b);
-    lib.sleep(sleepForMs);
+    sleep(sleepForMs);
     mutexCondvar.runLocked(() {
       resultIsReady = true;
       conditionVariable.notify();
@@ -175,7 +185,7 @@ Future<void> testNativeCallableHelloWorldClosure(NativeLibrary lib) async {
 
   result = 42;
   resultIsReady = false;
-  lib.callFunctionOnNewThreadNonBlocking(1001, callback.nativeFunction);
+  callFunctionOnNewThreadNonBlocking(1001, callback.nativeFunction);
 
   mutexCondvar.runLocked(() {
     while (!resultIsReady) {
@@ -186,7 +196,7 @@ Future<void> testNativeCallableHelloWorldClosure(NativeLibrary lib) async {
   Expect.equals(42 + (1001 * 123), result);
 
   resultIsReady = false;
-  lib.callFunctionOnNewThreadNonBlocking(1001, callback.nativeFunction);
+  callFunctionOnNewThreadNonBlocking(1001, callback.nativeFunction);
   mutexCondvar.runLocked(() {
     while (!resultIsReady) {
       conditionVariable.wait(mutexCondvar);
@@ -196,7 +206,7 @@ Future<void> testNativeCallableHelloWorldClosure(NativeLibrary lib) async {
   callback.close();
 }
 
-void testNativeCallableSync(NativeLibrary lib) {
+void testNativeCallableSync() {
   final callback =
       NativeCallable<CallbackReturningIntNativeType>.isolateGroupBound((
         int a,
@@ -205,14 +215,11 @@ void testNativeCallableSync(NativeLibrary lib) {
         return a + b;
       }, exceptionalReturn: 1111);
 
-  Expect.equals(
-    1234,
-    lib.callTwoIntFunction(callback.nativeFunction, 1000, 234),
-  );
+  Expect.equals(1234, callTwoIntFunction(callback.nativeFunction, 1000, 234));
   callback.close();
 }
 
-void testNativeCallableSyncThrows(NativeLibrary lib) {
+void testNativeCallableSyncThrows() {
   final callback =
       NativeCallable<CallbackReturningIntNativeType>.isolateGroupBound(
         (int a, int b) {
@@ -222,16 +229,13 @@ void testNativeCallableSyncThrows(NativeLibrary lib) {
         exceptionalReturn: 1111,
       );
 
-  Expect.equals(
-    1111,
-    lib.callTwoIntFunction(callback.nativeFunction, 1000, 234),
-  );
+  Expect.equals(1111, callTwoIntFunction(callback.nativeFunction, 1000, 234));
   callback.close();
 }
 
 int isolateVar = 10;
 
-void testNativeCallableAccessNonSharedVar(NativeLibrary lib) {
+void testNativeCallableAccessNonSharedVar() {
   final callback =
       NativeCallable<CallbackReturningIntNativeType>.isolateGroupBound((
         int a,
@@ -241,10 +245,7 @@ void testNativeCallableAccessNonSharedVar(NativeLibrary lib) {
       }, exceptionalReturn: 1111);
 
   isolateVar = 42;
-  Expect.equals(
-    1111,
-    lib.callTwoIntFunction(callback.nativeFunction, 1000, 234),
-  );
+  Expect.equals(1111, callTwoIntFunction(callback.nativeFunction, 1000, 234));
   callback.close();
 }
 
@@ -304,14 +305,13 @@ Future<void> testKeepIsolateAliveFalse() async {
 main(args, message) async {
   asyncStart();
   // Simple tests.
-  final ffiTestFunctions = dlopenPlatformSpecific("ffi_test_functions");
-  final lib = NativeLibrary(ffiTestFunctions);
-  await testNativeCallableHelloWorld(lib);
-  await testNativeCallableThrows(lib);
-  await testNativeCallableHelloWorldClosure(lib);
-  testNativeCallableSync(lib);
-  testNativeCallableSyncThrows(lib);
-  testNativeCallableAccessNonSharedVar(lib);
+  await testNativeCallableHelloWorld();
+  await testNativeCallableThrows();
+  await testFailToCaptureReceivePort();
+  await testNativeCallableHelloWorldClosure();
+  testNativeCallableSync();
+  testNativeCallableSyncThrows();
+  testNativeCallableAccessNonSharedVar();
   await testKeepIsolateAliveTrue();
   await testKeepIsolateAliveFalse();
   asyncEnd();

tests/ffi/isolate_group_bound_send_test.dart

@@ -21,10 +21,15 @@ import "package:expect/expect.dart";
 main() async {
   asyncStart();
   ReceivePort rp = ReceivePort();
-  IsolateGroup.runSync(() {
-    rp.sendPort.send("hello");
-  });
-  Expect.equals("hello", await rp.first);
+  Expect.throws(
+    () {
+      IsolateGroup.runSync(() {
+        rp.sendPort.send("hello");
+      });
+    },
+    (e) =>
+        e is ArgumentError && e.toString().contains('Only trivially-immutable'),
+  );
   rp.close();
   asyncEnd();
 }