[vm/ffi] Struct.addressOf removal

[dcharkes]

Update 2021-01-27: We have deprecated .addressOf for Dart 2.12 and will remove it in Dart 2.13.

Structs do not expose whether they are backed by c memory (Pointer) or Dart memory (TypedData). If interaction with c memory is required, the pointer to the struct should be kept around.

=========

Currently we support doing .addressOf on Structs.

sdk/sdk/lib/ffi/struct.dart

Lines 34 to 38 in a75ffc8

	/// Extension on [Struct] specialized for it's subtypes.
	extension StructAddressOf<T extends Struct> on T {
	/// Returns the address backing the reference.
	Pointer<T> get addressOf => _addressOf as Pointer<T>;
	}

The address returned is a different Pointer object, possibly with a different address if an index was passed:

sdk/sdk/lib/ffi/ffi.dart

Lines 607 to 612 in a75ffc8

	/// Creates a reference to access the fields of this struct backed by native
	/// memory at `address + sizeOf<T>() * index`.
	///
	/// The [address] must be aligned according to the struct alignment rules of
	/// the platform.
	external T operator [](int index);

This logic no longer works when adding finalizers, as finalizers depend on object identity.

We have multiple options here:

1. Return the original backing Pointer without the byte offset, which would be the wrong address if an index was passed. This would be a breaking change.
2. Return a derived Pointer, documenting that users cannot rely on finalizers when doing this. This would definitely be a footgun.
3. Remove .addressOf all together. This is also a breaking change. (There are 70 uses in our code base alone, excluding packages. It would require changing dart code to pass around Pointer<MyStruct> instead of MyStruct in many places.)

Side note, in order for finalizer behavior to work correctly the Struct class needs to use the original Pointer to access the fields:

abstract class Struct extends NativeType {
  final Pointer<Struct> _addressOf;
  final int _byteOffset; // 0 if `index` was 0.

  // some field `@Int8() a` access
  int get a => _loadInt8(_addressOf, _byteOffset + _a_offset);

(Currently, there's no _byteOffset field, and the _addressOf contains a derived pointer.)

Related issues:

• Finalizers: dart:ffi GC finalizers #35770
• Treeshaking: [vm/ffi] Support treeshaking of FFI structs #38721

cc @mkustermann

[ds84182]

I think Pointer itself shouldn't have associated identity, because in the future you'd want to have unboxed pointers (right?).

Maybe the finalizer can be associated with a separate object (maybe like a Managed<T extends NativeType>, maybe with a better name)? Then you can safely decay the object into a pointer .asPointer and retain all the normal semantics you would expect a pointer to have.

Temporary pointers (like indexing into arrays) shouldn't need to handle the burden of finalizers, but (some) allocations do.

[dcharkes]

Maybe the finalizer can be associated with a separate object

If we associate the finalizer with a separate object (not the Pointer that one uses to access the memory), people would need to always pass around both the pointer and the separate object in order to avoid early finalization, doesn't that defeat the purpose of finalizers? Or did you mean something else?

[ds84182]

An example:

class Managed<T extends NativeType> {
  Managed._(); // Cannot be constructed by user code
  Pointer<T> _pointer; // Internally contains a pointer
  Pointer<T> get asPointer => _pointer; // Exposes the pointer to user code to use (temporarily)
  void removeFinalizer(); // Removes the finalizer, useful for times where user code manually requests a cleanup
}

// Creates a instance of Managed, which has identity and a finalizer
Managed<T> addFinalizer(Pointer<T> ptr, Pointer<NativeFunction<void Function(Pointer<T>)>> free);

Then when you use this new object:

final array = addFinalizer(allocate<Uint64>(count: 8), free);
array.asPointer[0] = 123; // With asPointer
array[1] = 456; // With an extension method that uses the underlying pointer

The thing that should be made clear is that Managed<T> is not a normal Pointer<T>. Otherwise, if finalizers are attached to a Pointer<T> then putting pointers inside of a struct is an easy way to cause a use-after-free:

class Uint64Array extends Struct {
  Pointer<Uint64> values;
  IntPtr size;
}

Pointer<Uint64Array> newArray(int length) {
  final array = addFinalizer(allocate<Uint64Array>(), free);
  array.ref.values = addFinalizer(allocate<Uint64>(count: length), free); // Bug here, since object identity is being discarded during the struct write
  array.ref.size = length;
  return array;
}

[dcharkes]

// Exposes the pointer to user code to use (temporarily)

This is a problem. All your code will have to look like:

Managed<Int8> m; // has a finalizer
f(m.asPointer);
reachabilityFence(m);

Otherwise m can get garbage collected before running f():

Managed<Int8> m; // has a finalizer
final value1 = m.asPointer;
// runs garbage collector, sees `m` is unreachable.
f(value1);

Here is an older issue which explored a similar idea, but with subtypes of Pointer: #35841.

The thing that should be made clear is that Managed is not a normal Pointer. Otherwise, if finalizers are attached to a Pointer then putting pointers inside of a struct is an easy way to cause a use-after-free

Any place where you pass a pointer to C need a reachability fence:

• store into memory (either through struct or not).
• pass through an ffi call, and holding on to it on the C side after returning from the call.

For structs we envision people writing allocate/free C functions for the data structure. That way you'd only have one C function to call, and only on the outermost Dart object representing the struct. For example, SQLite has this pattern:

sdk/samples/ffi/sqlite/lib/src/bindings/bindings.dart

Line 213 in 81d4cc6

int Function(Pointer<Statement> statement) sqlite3_finalize;

[dcharkes]

Notes from a discussion today (@mkustermann @mraleph):

When we add structs by value (#36730), if we use internal TypedData, asPointer would have to throw on those structs. Because we cannot hand out pointer to things in the Dart heap that will be relocated by the GC.

When we add structs by value (#36730), if we use ExternalTypedData, asPointer would work properly. However, it would be slower due to having to malloc and free with a finalizer.

When we add nested structs (#37271), asPointer is the only way to get a pointer to the inner struct at the moment. The current api does not expose a way to get to an inner Pointer on the Pointer itself:

class MyInnerStruct extends Struct {
  @Int8() int;
}
class MyStruct extends Struct {
  MyInnerStruct inner;
}

Pointer<MyStruct> p;
Pointer<MyInnerStruct> p2 = p.ref.inner.asPointer;

[ds84182]

Categorically, it seems that there are several different types of pointers here:

By-value structs using TypedData, pointer references to TypedData, and pointers with finalizers should be treated as opaque by Dart, since they deal with "external forces" (GC). These all deal with GC-managed resources and therefore not safe to modify as raw pointers from Dart. We could call these managed pointers.

Nested structs pointers, elementAt, and pointers returned from FFI (through function calls or struct fields) aren't opaque to Dart, since the underlying data isn't allowed to move. We can also call these unmanaged pointers.

Most of the complexity comes from getting unmanaged pointers from managed pointers. Is it possible to pin managed pointers while unmanaged pointers are alive?

[dcharkes]

Most of the complexity comes from getting unmanaged pointers from managed pointers.

In this design, all Pointers are un-relocatable. However, subtypes of Struct can be either relocatable (when backed by TypedData) or un-relocatable (when backed by ExternalTypedData or Pointer).

Pointer, Struct backed by Pointer, and Struct backed by external typed data:

• malloced memory, either in C, or via Dart external typed data.
• Cannot be moved by Dart GC.
• Getting the int address out and doing something with that is safe.
• Doing all other operations in the FFI API is safe. (Edit: as long as Pointer is kept alive if there's a finalizer attached. Thanks for noting @ds84182!)
• Users can add finalizers to these, especially when malloced through the FFI. (External typed data will have an internal finalizer associated with it already.)

Struct backed by internal typed data:

• memory in the Dart heap.
• Can be moved by Dart GC.
• Getting the int address out is unsafe. (Hence, asPointer should throw.)
• Doing all other operations in the FFI API is safe.
• Users should never add finalizers to these. (Which they cant because that's an operation on Pointer, not Struct, and asPointer throws.)

I agree it is not desirable that we cannot see the difference between relocatable and un-relocatable objects statically in the types. Let's see if we can come up with something to address that.

Is it possible to pin managed pointers while unmanaged pointers are alive?

Great idea, unfortunately, the Dart GC does not have a feature to pin objects to prevent them from moving.

[ds84182]

My only nit is that Getting the int address out and doing something with that is safe isn't valid in the case where there is a finalizer, but only if the original Pointer becomes unreachable by the GC, which is technically "getting moved by the Dart GC". Except it's more like its being moved into /dev/null. Either way, the original pointer is now invalid. Whether or not there is a new allocation that represents it is up to what's being moved, if its still reachable.

The same thing can happen with structs backed by internal typed data, if you count object destruction as "getting moved by the GC".

This goes back to the original "non-Pointer object that represents the allocation w/ finalization" idea.

[dcharkes]

My only nit is that Getting the int address out and doing something with that is safe isn't valid in the case where there is a finalizer, but only if the original Pointer becomes unreachable by the GC

True, good catch!

Design

I think in general I think it'd better to expose to users that it can be either Pointer or TypedData.

Current design https://dart-review.googlesource.com/c/sdk/+/137581.

We'll probably replace addressOf with backingStore and offsetInBytes which is finalizer-safe.
(With those people can write their own extension method to make addressOf again, which will not be finalizer-safe.)

abstract class Struct extends NativeType {
  final Pointer<Struct> _backingStore;  // Can also be typed data later when we add structs by value.
  final int _offsetInBytes;

  /// Construct a reference to the [nullptr].
  ///
  /// Use [StructPointer]'s `.ref` to gain references to native memory backed
  /// structs.
  Struct()
      : _backingStore = nullptr,
        _offsetInBytes = 0;

  Struct._fromPointer(this._backingStore, this._offsetInBytes);
}

/// Extension on [Struct] specialized for it's subtypes.
extension StructAddressOf<T extends Struct> on T {
  /// Returns the address backing the reference.
  ///
  /// Deprecated, see https://github.com/dart-lang/sdk/issues/40667.
  @deprecated
  Pointer<T> get addressOf {
    if (_offsetInBytes == 0) {
      return _backingStore as Pointer<T>;
    }
    return Pointer<T>.fromAddress(_backingStore.address + _offsetInBytes);
  }
}

[a-siva]

CL was reverted so reopening the issue.