[Breaking Change Request] [vm/ffi] Disallow WeakHandle and FinalizableHandle on Pointers and Structs

[dcharkes]

Summary

Disallow invoking Dart_NewWeakPersistentHandle and Dart_NewFinalizableHandle in dart_api.h with object without identity.

This is already already true for integers, true, and false, but we'd like to extend it to dart:ffi's Pointer, and subtypes of dart:ffi's Struct.

We also disallow using Pointer, and subtypes of dart:ffi's Struct in Expandos, similar to how numbers, booleans, and null are disallowed.

sdk/runtime/include/dart_api.h

Lines 460 to 494 in 8f0d5ef

	/**
	* Allocates a weak persistent handle for an object.
	*
	* This handle has the lifetime of the current isolate. The handle can also be
	* explicitly deallocated by calling Dart_DeleteWeakPersistentHandle.
	*
	* If the object becomes unreachable the callback is invoked with the peer as
	* argument. The callback can be executed on any thread, will have a current
	* isolate group, but will not have a current isolate. The callback can only
	* call Dart_DeletePersistentHandle or Dart_DeleteWeakPersistentHandle. This
	* gives the embedder the ability to cleanup data associated with the object.
	* The handle will point to the Dart_Null object after the finalizer has been
	* run. It is illegal to call into the VM with any other Dart_* functions from
	* the callback. If the handle is deleted before the object becomes
	* unreachable, the callback is never invoked.
	*
	* Requires there to be a current isolate.
	*
	* \param object An object.
	* \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
	* bytes for peer. Used to inform the garbage collector.
	* \param callback A function pointer that will be invoked sometime
	* after the object is garbage collected, unless the handle has been deleted.
	* A valid callback needs to be specified it cannot be NULL.
	*
	* \return The weak persistent handle or NULL. NULL is returned in case of bad
	* parameters.
	*/
	DART_EXPORT Dart_WeakPersistentHandle
	Dart_NewWeakPersistentHandle(Dart_Handle object,
	void* peer,
	intptr_t external_allocation_size,
	Dart_HandleFinalizer callback);

sdk/runtime/include/dart_api.h

Lines 512 to 550 in 8f0d5ef

	/**
	* Allocates a finalizable handle for an object.
	*
	* This handle has the lifetime of the current isolate group unless the object
	* pointed to by the handle is garbage collected, in this case the VM
	* automatically deletes the handle after invoking the callback associated
	* with the handle. The handle can also be explicitly deallocated by
	* calling Dart_DeleteFinalizableHandle.
	*
	* If the object becomes unreachable the callback is invoked with the
	* the peer as argument. The callback can be executed on any thread, will have
	* an isolate group, but will not have a current isolate. The callback can only
	* call Dart_DeletePersistentHandle or Dart_DeleteWeakPersistentHandle.
	* This gives the embedder the ability to cleanup data associated with the
	* object and clear out any cached references to the handle. All references to
	* this handle after the callback will be invalid. It is illegal to call into
	* the VM with any other Dart_* functions from the callback. If the handle is
	* deleted before the object becomes unreachable, the callback is never
	* invoked.
	*
	* Requires there to be a current isolate.
	*
	* \param object An object.
	* \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
	* bytes for peer. Used to inform the garbage collector.
	* \param callback A function pointer that will be invoked sometime
	* after the object is garbage collected, unless the handle has been deleted.
	* A valid callback needs to be specified it cannot be NULL.
	*
	* \return The finalizable handle or NULL. NULL is returned in case of bad
	* parameters.
	*/
	DART_EXPORT Dart_FinalizableHandle
	Dart_NewFinalizableHandle(Dart_Handle object,
	void* peer,
	intptr_t external_allocation_size,
	Dart_HandleFinalizer callback);

Motivation

1. We want to be able to optimize Pointer and subtypes of Struct away completely at runtime.
2. We want to prevent use after free issues with the GC running finalizers on these objects when derived pointers or the pointer's addresses are still in use. (Example in [vm/ffi] Disallow adding finalizers to Pointer #45071.)

Impact

We're not aware of any uses of finalizers on Pointers or structs. (If there are any, they might already suffer from use-after-free bugs due to premature finalization.)

Migration

void myCode() {
  Pointer<Database> pointer = // ...
  // Pass `pointer` to native code with `dart:ffi` and call `Dart_NewFInalizableHandle` to attach finalizer.
  use(pointer);
}

becomes

class DatabaseResource implements NativeResource {
  final Pointer<Database> _database;

  DatabaseResource(this._database) {
    // Pass `this` and `_database` to native code with `dart:ffi` and call `Dart_NewFInalizableHandle`
    // with `this` as parameter to `object` and `_database` as `peer`.
  }

  void use();
}

void myCode() {
  DatabaseResource database;
  database.use();
}

https://dart-review.googlesource.com/c/sdk/+/143804

Future work

Ensuring that classes implementing this this NativeResource/WeakHandleable interface are kept alive on all method calls is future work (design discussion).

[franklinyow]

CC: @mit-mit @vsmenon @kevmoo @Hixie @matanlurey

[matanlurey]

Not relevant to my work, so no opinion either way.

[jonasfj]

Hmm, attaching finalizers to the Pointer<..> object is very powerful when you have a C library with referencing.

Example: In package:webcrypto I create a Pointer<EVP_KEY> then attach a finalizer with Dart_NewFinalizableHandle_DL and because every function that uses the EVP_KEY pointer takes it as an argument and either:

1. Doesn't hold a reference to it, OR,
2. Increases a reference count in the C library,

I do not need to insert artificial use(pkey) calls all over my code, nor do I need anything like NativeResource.

---

I can certainly see the motivation for this, I just wanted to point out that finalizers on Pointer is attractive for use with C libraries that have reference counting. Whether feasible to allow that or not I don't know.

[franklinyow]

ping: @mit-mit @vsmenon @kevmoo @Hixie for approval

[Hixie]

no opinion

[dcharkes]

We've added rejecting Pointers and Structs in Expandos to this breaking change request.

Ping @mit-mit, @vsmenon, and @kevmoo for approval.

[franklinyow]

Ping @mit-mit, @vsmenon, and @kevmoo for approval.

[leafpetersen]

I believe this has slipped from this milestone.