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

dart:ffi accessible via mirrors even when --enable-ffi=false #37044

Closed
mdempsky opened this issue May 21, 2019 · 5 comments
Closed

dart:ffi accessible via mirrors even when --enable-ffi=false #37044

mdempsky opened this issue May 21, 2019 · 5 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@mdempsky
Copy link
Contributor

It looks like the --enable-ffi flag simply prevents importing "dart:ffi", whereas the library still exists within the VM and is accessible via mirrors. For example:

$ cat x.dart
import "dart:mirrors";

void main() {
  var libs = currentMirrorSystem().libraries;
  var ffi = libs[Uri(scheme: "dart", path: "ffi")];
  print(ffi);
}

$ dart --enable-ffi=false x.dart
LibraryMirror on 'dart.ffi'

It seems like --enable-ffi (and --enable-mirrors) should operate by preventing the VM from constructing and initializing the underlying ffi (or mirrors) Library entirely.

/cc @sjindel-google

@mdempsky mdempsky added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label May 21, 2019
@sjindel-google sjindel-google added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label May 21, 2019
@sjindel-google
Copy link
Contributor

I think we should disable the entrypoints from the FFI library into the VM (runtime/lib/ffi.cc, e.g. they could throw an exception) when the FFI is not enabled. The upper (Dart) half of the FFI library is useless if the entrypoints to the VM are disabled.

@mdempsky
Copy link
Contributor Author

@sjindel-google Agreed that the Dart upper half seems harmless without the native entrypoints lower half, so that sounds reasonable to me too. Out of curiosity though, what do you see as the benefit to that approach vs avoiding creating the library entirely?

@sjindel-google
Copy link
Contributor

IIRC there were some technical issues which made removing the library inconvenient (maybe due to the way loading from Kernel works). However, we should implement both solutions if possible to be safe.

@mdempsky
Copy link
Contributor Author

It looks like #33594 unintentionally mitigates this problem currently: when using dart:ffi via dart:mirrors, I can't find a way to explicitly supply type arguments, so the type parameters all fall back to their default NativeType, which gets rejected by runtime/lib/ffi.c's CheckIsConcreteNativeType.

@rmacnak-google
Copy link
Contributor

FWIW, the mirrors implementation already has a notion of censored libraries. It should be easy to censor dart:ffi when FFI is disabled in CreateLibraryMirror.

dart-bot pushed a commit that referenced this issue Jun 6, 2019
Updates #37044.

Change-Id: I51b11fe8f326e1f98d86fc674147176afae873a9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/105327
Commit-Queue: Matthew Dempsky <mdempsky@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Samir Jindel <sjindel@google.com>
Reviewed-by: Ryan Macnak <rmacnak@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 type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

4 participants