Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[vm/ffi] Disallow adding finalizers to Pointer #45071

Closed
dcharkes opened this issue Feb 22, 2021 · 3 comments
Closed

[vm/ffi] Disallow adding finalizers to Pointer #45071

dcharkes opened this issue Feb 22, 2021 · 3 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi

Comments

@dcharkes
Copy link
Contributor

We should disallow Pointer in Dart_NewWeakPersistentHandle and Dart_NewFinalizableHandle.

  1. Pointers do not have identity. We might want to optimize them to just be integers later.

  2. Also the nested structs and inline arrays representation does not keep original pointers alive:

class Foo extends Struct {
  external Bar bar;
}
class Bar extends Struct {
  Int32()
  external int a;
}

void main(){
  Pointer<Foo> fooPointer = calloc();
  // Attach native finalizer to `fooPointer` using `Dart_NewWeakPersistentHandle` or `Dart_NewFinalizableHandle`.
  Foo foo = fooPointer.ref;
  Bar bar = foo.bar;
  // GC
  bar.a; // undefined behavior.
}

This is a breaking change.

@dcharkes dcharkes added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Feb 22, 2021
@mkustermann
Copy link
Member

Maybe we should make a breaking change request following docs/process/breaking-changes.md and getting it approved?

Furthermore: One reason for doing this breaking change is to avoid use-after-free if pointer gets finalized before derived objects. The same argument can be made for struct objects which might get finalized before derived objects (e.g. inner structs / arrays). So I think we should include that in this breaking change request.

@simolus3
Copy link
Contributor

Pointers do not have identity. We might want to optimize them to just be integers later.

This might have to be another (breaking?) issue, but I think that an Expando should also reject pointers if they have no identity? Should they also be disallowed in identical and identityHashCode?

static _checkType(object) {
if ((object == null) ||
(object is bool) ||
(object is num) ||
(object is String)) {
throw new ArgumentError.value(object,
"Expandos are not allowed on strings, numbers, booleans or null");
}
}

@dcharkes
Copy link
Contributor Author

This might have to be another (breaking?) issue, but I think that an Expando should also reject pointers if they have no identity?

After some internal discussion, we've added this to the breaking change request.

Should they also be disallowed in identical and identityHashCode?

After some internal discussion, we'll keep these as they are for now. (See https://dart-review.googlesource.com/c/sdk/+/186280/11..16/CHANGELOG.md#b11 on telling yourself apart from identical twins. 😄 )

Thanks for your suggestions @simolus3!

@dcharkes dcharkes reopened this Apr 12, 2021
@dcharkes dcharkes reopened this Apr 12, 2021
dart-bot pushed a commit that referenced this issue Apr 13, 2021
…s and expandos""

This reverts commit 3f0ad61.

Reason for revert: Conflicts with https://dart-review.googlesource.com/c/sdk/+/194765

Original change's description:
> Reland "[vm/ffi] Disallow `Pointer`s 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: #45071
> Breaking change: #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>

TBR=kustermann@google.com,dacoharkes@google.com

Change-Id: I90064b6b73155a43f37388f987c6b29f72ce9770
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/195076
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
dart-bot pushed a commit that referenced this issue Apr 14, 2021
…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.

Reland 1: Allow importing `dart:ffi` from `dart:core` with
          `--enable-ffi=false`.
Reland 2: Allow the new `_Compound` class to extend `NativeType`. (This
          got triggered in this CL on the build because of the import
          from `dart:core`.)

Closes: #45071
Breaking change: #45072

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

Change-Id: I133278e16bd75cd2bb6234e7ddf042ffa0a54fd6
Cq-Include-Trybots: luci.dart.try:dart-sdk-linux-try,dart-sdk-mac-try,dart-sdk-win-try,vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-mac-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-linux-debug-x64-try,vm-kernel-nnbd-linux-debug-x64-try,vm-kernel-nnbd-linux-debug-ia32-try,vm-kernel-nnbd-mac-release-x64-try,vm-kernel-nnbd-win-debug-x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-linux-debug-simarm_x64-try,vm-kernel-precomp-nnbd-linux-debug-x64-try,vm-kernel-precomp-win-release-x64-try,vm-kernel-reload-linux-debug-x64-try,vm-kernel-reload-rollback-linux-debug-x64-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,vm-kernel-msan-linux-release-x64-try,vm-kernel-precomp-msan-linux-release-x64-try,vm-kernel-precomp-android-release-arm_x64-try,analyzer-analysis-server-linux-try,vm-kernel-precomp-linux-debug-x64c-try,vm-kernel-linux-debug-x64c-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/195079
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi
Projects
None yet
Development

No branches or pull requests

3 participants