Reland "[vm/ffi] Disallow Pointers and structs in finalizers and expandos"

`Dart_NewWeakPersistentHandle` and `Dart_NewFinalizableHandle` in
`dart_api.h` do no longer accept `Pointer`s and subtypes of `Struct`s
as values passed in to the `object` parameter.

Expandos no longer accept `Pointer`s and subtypes of `Struct`s
as values passed in to the `object` parameter.

Cleans up unused object_store->ffi_struct_class.

Closes: https://github.com/dart-lang/sdk/issues/45071
Breaking change: https://github.com/dart-lang/sdk/issues/45072

TEST=vm/cc/DartAPI_FinalizableHandleErrors
TEST=vm/cc/DartAPI_WeakPersistentHandleErrors
TEST=tests/ffi(_2)/expando_test.dart

Change-Id: I9af6d0173db60614091068c218391f73756c135f
Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/195061
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>

CHANGELOG.md

@@ -8,6 +8,15 @@
     daylight saving changes that are not precisely one hour.
     (No change on the Web which uses the JavaScript `Date` object.)
 
+### Dart VM
+
+*   **Breaking Change** [#45071][]: `Dart_NewWeakPersistentHandle`'s and
+    `Dart_NewFinalizableHandle`'s `object` parameter no longer accepts
+    `Pointer`s and subtypes of `Struct`. Expandos no longer accept
+    `Pointer`s and subtypes of `Struct`s.
+
+[#45071]: https://github.com/dart-lang/sdk/issues/45071
+
 ## 2.13.0
 
 ### Language

pkg/vm/lib/transformations/ffi_use_sites.dart

@@ -834,7 +834,9 @@ class _FfiUseSiteTransformer extends FfiTransformer {
           : null;
     }
 
-    if (!nativeTypesClasses.contains(klass) && klass != arrayClass) {
+    if (!nativeTypesClasses.contains(klass) &&
+        klass != arrayClass &&
+        klass != arraySizeClass) {
       for (final parent in nativeTypesClasses) {
         if (hierarchy.isSubtypeOf(klass, parent)) {
           return parent;

runtime/include/dart_api.h

@@ -475,7 +475,7 @@ DART_EXPORT void Dart_DeletePersistentHandle(Dart_PersistentHandle object);
  *
  * Requires there to be a current isolate.
  *
- * \param object An object.
+ * \param object An object with identity.
  * \param peer A pointer to a native object or NULL.  This value is
  *   provided to callback when it is invoked.
  * \param external_allocation_size The number of externally allocated
@@ -531,7 +531,7 @@ DART_EXPORT void Dart_UpdateExternalSize(Dart_WeakPersistentHandle object,
  *
  * Requires there to be a current isolate.
  *
- * \param object An object.
+ * \param object An object with identity.
  * \param peer A pointer to a native object or NULL.  This value is
  *   provided to callback when it is invoked.
  * \param external_allocation_size The number of externally allocated

runtime/vm/dart_api_impl.cc

@@ -991,6 +991,23 @@ DART_EXPORT void Dart_SetPersistentHandle(Dart_PersistentHandle obj1,
   obj1_ref->set_ptr(obj2_ref);
 }
 
+// TODO(https://dartbug.com/38491): Reject Unions here as well.
+static bool IsFfiStruct(Thread* T, const Object& obj) {
+  if (obj.IsNull()) {
+    return false;
+  }
+
+  // CFE guarantees we can only have direct subclasses of `Struct`
+  // (no implementations or indirect subclasses are allowed).
+  const auto& klass = Class::Handle(Z, obj.clazz());
+  const auto& super_klass = Class::Handle(Z, klass.SuperClass());
+  if (super_klass.Name() != Symbols::Struct().ptr()) {
+    return false;
+  }
+  const auto& library = Library::Handle(Z, super_klass.library());
+  return library.url() == Symbols::DartFfi().ptr();
+}
+
 static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
     Thread* thread,
     const Object& ref,
@@ -1000,6 +1017,13 @@ static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
   if (!ref.ptr()->IsHeapObject()) {
     return NULL;
   }
+  if (ref.IsPointer()) {
+    return NULL;
+  }
+  if (IsFfiStruct(thread, ref)) {
+    return NULL;
+  }
+
   FinalizablePersistentHandle* finalizable_ref =
       FinalizablePersistentHandle::New(thread->isolate_group(), ref, peer,
                                        callback, external_allocation_size,
@@ -1008,16 +1032,14 @@ static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
 }
 
 static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
-    Thread* thread,
+    Thread* T,
     Dart_Handle object,
     void* peer,
     intptr_t external_allocation_size,
     Dart_HandleFinalizer callback) {
-  REUSABLE_OBJECT_HANDLESCOPE(thread);
-  Object& ref = thread->ObjectHandle();
-  ref = Api::UnwrapHandle(object);
-  return AllocateWeakPersistentHandle(thread, ref, peer,
-                                      external_allocation_size, callback);
+  const auto& ref = Object::Handle(Z, Api::UnwrapHandle(object));
+  return AllocateWeakPersistentHandle(T, ref, peer, external_allocation_size,
+                                      callback);
 }
 
 static Dart_FinalizableHandle AllocateFinalizableHandle(
@@ -1029,6 +1051,12 @@ static Dart_FinalizableHandle AllocateFinalizableHandle(
   if (!ref.ptr()->IsHeapObject()) {
     return NULL;
   }
+  if (ref.IsPointer()) {
+    return NULL;
+  }
+  if (IsFfiStruct(thread, ref)) {
+    return NULL;
+  }
 
   FinalizablePersistentHandle* finalizable_ref =
       FinalizablePersistentHandle::New(thread->isolate_group(), ref, peer,
@@ -1038,15 +1066,13 @@ static Dart_FinalizableHandle AllocateFinalizableHandle(
 }
 
 static Dart_FinalizableHandle AllocateFinalizableHandle(
-    Thread* thread,
+    Thread* T,
     Dart_Handle object,
     void* peer,
     intptr_t external_allocation_size,
     Dart_HandleFinalizer callback) {
-  REUSABLE_OBJECT_HANDLESCOPE(thread);
-  Object& ref = thread->ObjectHandle();
-  ref = Api::UnwrapHandle(object);
-  return AllocateFinalizableHandle(thread, ref, peer, external_allocation_size,
+  const auto& ref = Object::Handle(Z, Api::UnwrapHandle(object));
+  return AllocateFinalizableHandle(T, ref, peer, external_allocation_size,
                                    callback);
 }
 
@@ -1055,15 +1081,13 @@ Dart_NewWeakPersistentHandle(Dart_Handle object,
                              void* peer,
                              intptr_t external_allocation_size,
                              Dart_HandleFinalizer callback) {
-  Thread* thread = Thread::Current();
-  CHECK_ISOLATE(thread->isolate());
+  DARTSCOPE(Thread::Current());
   if (callback == NULL) {
     return NULL;
   }
-  TransitionNativeToVM transition(thread);
 
-  return AllocateWeakPersistentHandle(thread, object, peer,
-                                      external_allocation_size, callback);
+  return AllocateWeakPersistentHandle(T, object, peer, external_allocation_size,
+                                      callback);
 }
 
 DART_EXPORT Dart_FinalizableHandle
@@ -1071,14 +1095,13 @@ Dart_NewFinalizableHandle(Dart_Handle object,
                           void* peer,
                           intptr_t external_allocation_size,
                           Dart_HandleFinalizer callback) {
-  Thread* thread = Thread::Current();
-  CHECK_ISOLATE(thread->isolate());
+  DARTSCOPE(Thread::Current());
   if (callback == nullptr) {
     return nullptr;
   }
-  TransitionNativeToVM transition(thread);
-  return AllocateFinalizableHandle(thread, object, peer,
-                                   external_allocation_size, callback);
+
+  return AllocateFinalizableHandle(T, object, peer, external_allocation_size,
+                                   callback);
 }
 
 DART_EXPORT void Dart_UpdateExternalSize(Dart_WeakPersistentHandle object,

runtime/vm/dart_api_impl_test.cc

@@ -3368,6 +3368,35 @@ TEST_CASE(DartAPI_WeakPersistentHandleErrors) {
       Dart_NewWeakPersistentHandle(obj2, NULL, 0, NopCallback);
   EXPECT_EQ(ref2, static_cast<void*>(NULL));
 
+  // Pointer object.
+  Dart_Handle ffi_lib = Dart_LookupLibrary(NewString("dart:ffi"));
+  Dart_Handle pointer_type =
+      Dart_GetNonNullableType(ffi_lib, NewString("Pointer"), 0, NULL);
+  Dart_Handle obj3 = Dart_Allocate(pointer_type);
+  EXPECT_VALID(obj3);
+  Dart_WeakPersistentHandle ref3 =
+      Dart_NewWeakPersistentHandle(obj3, nullptr, 0, FinalizableHandleCallback);
+  EXPECT_EQ(ref3, static_cast<void*>(nullptr));
+
+  // Subtype of Struct object.
+  const char* kScriptChars = R"(
+      import 'dart:ffi';
+
+      class MyStruct extends Struct {
+        external Pointer notEmpty;
+      }
+  )";
+  Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, NULL);
+  Dart_Handle my_struct_type =
+      Dart_GetNonNullableType(lib, NewString("MyStruct"), 0, NULL);
+  Dart_Handle obj4 = Dart_Allocate(my_struct_type);
+  EXPECT_VALID(obj4);
+  Dart_WeakPersistentHandle ref4 =
+      Dart_NewWeakPersistentHandle(obj4, nullptr, 0, FinalizableHandleCallback);
+  EXPECT_EQ(ref4, static_cast<void*>(nullptr));
+
+  // TODO(https://dartbug.com/38491): Reject Unions here as well.
+
   Dart_ExitScope();
 }
 
@@ -3388,6 +3417,33 @@ TEST_CASE(DartAPI_FinalizableHandleErrors) {
       Dart_NewFinalizableHandle(obj2, nullptr, 0, FinalizableHandleCallback);
   EXPECT_EQ(ref2, static_cast<void*>(nullptr));
 
+  // Pointer object.
+  Dart_Handle ffi_lib = Dart_LookupLibrary(NewString("dart:ffi"));
+  Dart_Handle pointer_type =
+      Dart_GetNonNullableType(ffi_lib, NewString("Pointer"), 0, NULL);
+  Dart_Handle obj3 = Dart_Allocate(pointer_type);
+  EXPECT_VALID(obj3);
+  Dart_FinalizableHandle ref3 =
+      Dart_NewFinalizableHandle(obj3, nullptr, 0, FinalizableHandleCallback);
+  EXPECT_EQ(ref3, static_cast<void*>(nullptr));
+
+  // Subtype of Struct object.
+  const char* kScriptChars = R"(
+      import 'dart:ffi';
+
+      class MyStruct extends Struct {
+        external Pointer notEmpty;
+      }
+  )";
+  Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, NULL);
+  Dart_Handle my_struct_type =
+      Dart_GetNonNullableType(lib, NewString("MyStruct"), 0, NULL);
+  Dart_Handle obj4 = Dart_Allocate(my_struct_type);
+  EXPECT_VALID(obj4);
+  Dart_FinalizableHandle ref4 =
+      Dart_NewFinalizableHandle(obj4, nullptr, 0, FinalizableHandleCallback);
+  EXPECT_EQ(ref4, static_cast<void*>(nullptr));
+
   Dart_ExitScope();
 }
 

runtime/vm/kernel_loader.cc

@@ -1367,7 +1367,8 @@ void KernelLoader::LoadLibraryImportsAndExports(Library* library,
           "runtime");
     }
     if (!Api::IsFfiEnabled() &&
-        target_library.url() == Symbols::DartFfi().ptr()) {
+        target_library.url() == Symbols::DartFfi().ptr() &&
+        library->url() != Symbols::DartCore().ptr()) {
       H.ReportError(
           "import of dart:ffi is not supported in the current Dart runtime");
     }

runtime/vm/object_store.h

@@ -236,7 +236,6 @@ class ObjectPointerVisitor;
   RW(GrowableObjectArray, ffi_callback_functions)                              \
   RW(Class, ffi_pointer_class)                                                 \
   RW(Class, ffi_native_type_class)                                             \
-  RW(Class, ffi_struct_class)                                                  \
   RW(Object, ffi_as_function_internal)                                         \
   // Please remember the last entry must be referred in the 'to' function below.
 

sdk/lib/_internal/vm/lib/core_patch.dart

@@ -49,6 +49,8 @@ import "dart:collection"
 
 import "dart:convert" show ascii, Encoding, json, latin1, utf8;
 
+import "dart:ffi" show Pointer, Struct;
+
 import "dart:isolate" show Isolate;
 
 import "dart:math" show Random;

sdk/lib/_internal/vm/lib/expando_patch.dart

@@ -140,9 +140,12 @@ class Expando<T> {
     if ((object == null) ||
         (object is bool) ||
         (object is num) ||
-        (object is String)) {
+        (object is String) ||
+        (object is Pointer) ||
+        (object is Struct)) {
+      // TODO(https://dartbug.com/38491): Reject Unions here as well.
       throw new ArgumentError.value(object,
-          "Expandos are not allowed on strings, numbers, booleans or null");
+          "Expandos are not allowed on strings, numbers, booleans, null, Pointers or Structs.");
     }
   }
 

sdk/lib/core/expando.dart

@@ -6,7 +6,8 @@ part of dart.core;
 
 /// An [Expando] allows adding new properties to objects.
 ///
-/// Does not work on numbers, strings, booleans or `null`.
+/// Does not work on numbers, strings, booleans, `null`, `dart:ffi` pointers
+/// or `dart:ffi` structs.
 ///
 /// An `Expando` does not hold on to the added property value after an object
 /// becomes inaccessible.
@@ -38,13 +39,15 @@ class Expando<T extends Object> {
   /// object. If the object hasn't been expanded, the method returns
   /// `null`.
   ///
-  /// The object must not be a number, a string or a boolean.
+  /// The object must not be a number, a string, a boolean, `null`, a
+  /// `dart:ffi` pointer, or a `dart:ffi` struct.
   external T? operator [](Object object);
 
   /// Sets the value of this [Expando]'s property on the given
   /// object. Properties can effectively be removed again by setting
   /// their value to `null`.
   ///
-  /// The object must not be a number, a string or a boolean.
+  /// The object must not be a number, a string, a boolean, `null`, a
+  /// `dart:ffi` pointer, or a `dart:ffi` struct.
   external void operator []=(Object object, T? value);
 }

tests/corelib/expando_test.dart

@@ -1,6 +1,9 @@
 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
+//
+// VMOptions=--enable-ffi=true
+// VMOptions=--enable-ffi=false
 
 import "package:expect/expect.dart";
 

tests/corelib_2/expando_test.dart

@@ -1,6 +1,9 @@
 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
+//
+// VMOptions=--enable-ffi=true
+// VMOptions=--enable-ffi=false
 
 import "package:expect/expect.dart";
 

tests/ffi/expando_test.dart

@@ -0,0 +1,43 @@
+// Copyright (c) 2021, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+//
+// Dart test program for testing dart:ffi primitive data pointers.
+//
+// SharedObjects=ffi_test_functions
+
+import 'dart:ffi';
+
+import "package:expect/expect.dart";
+
+void main() {
+  testPointer();
+  testStruct();
+}
+
+Expando<int> expando = Expando('myExpando');
+
+void testPointer() {
+  final pointer = Pointer<Int8>.fromAddress(0xdeadbeef);
+  Expect.throws(() {
+    expando[pointer];
+  });
+  Expect.throws(() {
+    expando[pointer] = 1234;
+  });
+}
+
+class MyStruct extends Struct {
+  external Pointer notEmpty;
+}
+
+void testStruct() {
+  final pointer = Pointer<MyStruct>.fromAddress(0xdeadbeef);
+  final struct = pointer.ref;
+  Expect.throws(() {
+    expando[struct];
+  });
+  Expect.throws(() {
+    expando[struct] = 1234;
+  });
+}

tests/ffi_2/expando_test.dart

@@ -0,0 +1,43 @@
+// Copyright (c) 2021, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+//
+// Dart test program for testing dart:ffi primitive data pointers.
+//
+// SharedObjects=ffi_test_functions
+
+import 'dart:ffi';
+
+import "package:expect/expect.dart";
+
+void main() {
+  testPointer();
+  testStruct();
+}
+
+Expando<int> expando = Expando('myExpando');
+
+void testPointer() {
+  final pointer = Pointer<Int8>.fromAddress(0xdeadbeef);
+  Expect.throws(() {
+    expando[pointer];
+  });
+  Expect.throws(() {
+    expando[pointer] = 1234;
+  });
+}
+
+class MyStruct extends Struct {
+  Pointer notEmpty;
+}
+
+void testStruct() {
+  final pointer = Pointer<MyStruct>.fromAddress(0xdeadbeef);
+  final struct = pointer.ref;
+  Expect.throws(() {
+    expando[struct];
+  });
+  Expect.throws(() {
+    expando[struct] = 1234;
+  });
+}