diff --git a/CHANGELOG.md b/CHANGELOG.md index d627fa92bd0..36d142297d3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/pkg/vm/lib/transformations/ffi_use_sites.dart b/pkg/vm/lib/transformations/ffi_use_sites.dart index 20cfbe86ea3..bf144f4db10 100644 --- a/pkg/vm/lib/transformations/ffi_use_sites.dart +++ b/pkg/vm/lib/transformations/ffi_use_sites.dart @@ -835,7 +835,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; diff --git a/runtime/include/dart_api.h b/runtime/include/dart_api.h index b4415cff245..2643002cafc 100644 --- a/runtime/include/dart_api.h +++ b/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. Pointers do not have 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 diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc index 206aad7c013..a87928a224d 100644 --- a/runtime/vm/dart_api_impl.cc +++ b/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, diff --git a/runtime/vm/dart_api_impl_test.cc b/runtime/vm/dart_api_impl_test.cc index d77bbcf6151..6046c6e04b4 100644 --- a/runtime/vm/dart_api_impl_test.cc +++ b/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(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(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(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(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(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(nullptr)); + Dart_ExitScope(); } diff --git a/runtime/vm/object_store.h b/runtime/vm/object_store.h index a452fd35305..a0337d402ef 100644 --- a/runtime/vm/object_store.h +++ b/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. diff --git a/sdk/lib/_internal/vm/lib/core_patch.dart b/sdk/lib/_internal/vm/lib/core_patch.dart index 6ffdce8bf2a..d79e07dbb76 100644 --- a/sdk/lib/_internal/vm/lib/core_patch.dart +++ b/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; diff --git a/sdk/lib/_internal/vm/lib/expando_patch.dart b/sdk/lib/_internal/vm/lib/expando_patch.dart index 01cbe515749..868e6238e66 100644 --- a/sdk/lib/_internal/vm/lib/expando_patch.dart +++ b/sdk/lib/_internal/vm/lib/expando_patch.dart @@ -140,9 +140,12 @@ class Expando { 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."); } } diff --git a/sdk/lib/core/expando.dart b/sdk/lib/core/expando.dart index d623579d365..c081df80bc5 100644 --- a/sdk/lib/core/expando.dart +++ b/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 { /// 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); } diff --git a/tests/ffi/expando_test.dart b/tests/ffi/expando_test.dart new file mode 100644 index 00000000000..473fc6f0293 --- /dev/null +++ b/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 expando = Expando('myExpando'); + +void testPointer() { + final pointer = Pointer.fromAddress(0xdeadbeef); + Expect.throws(() { + expando[pointer]; + }); + Expect.throws(() { + expando[pointer] = 1234; + }); +} + +class MyStruct extends Struct { + external Pointer notEmpty; +} + +void testStruct() { + final pointer = Pointer.fromAddress(0xdeadbeef); + final struct = pointer.ref; + Expect.throws(() { + expando[struct]; + }); + Expect.throws(() { + expando[struct] = 1234; + }); +} diff --git a/tests/ffi_2/expando_test.dart b/tests/ffi_2/expando_test.dart new file mode 100644 index 00000000000..ac143245fbf --- /dev/null +++ b/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 expando = Expando('myExpando'); + +void testPointer() { + final pointer = Pointer.fromAddress(0xdeadbeef); + Expect.throws(() { + expando[pointer]; + }); + Expect.throws(() { + expando[pointer] = 1234; + }); +} + +class MyStruct extends Struct { + Pointer notEmpty; +} + +void testStruct() { + final pointer = Pointer.fromAddress(0xdeadbeef); + final struct = pointer.ref; + Expect.throws(() { + expando[struct]; + }); + Expect.throws(() { + expando[struct] = 1234; + }); +}