From 0310e41f2c9e86c5807ff98367f450bd13a080de Mon Sep 17 00:00:00 2001
From: Alexander Markov <alexmarkov@google.com>
Date: Fri, 1 Dec 2023 14:05:59 +0000
Subject: [PATCH] [vm/ffi] Verify NativeFunctionPointer.asFunction type
 arguments

Protect VM compiler code from crashing when incorrect
type arguments are passed to FFI _asFunctionInternal.

Also, improve front-end FFI checks to reject Function
as a Dart type corresponding to a NativeFunction<...>
(it should be an actual function type).

TEST=tests/ffi/vmspecific_static_checks_test.dart
Fixes https://github.com/dart-lang/sdk/issues/52730

Change-Id: I02c012bb76d5c94f551da2c725a476b68c0f9db4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/339109
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
---
 pkg/vm/lib/transformations/ffi/common.dart    | 16 +++++++++++-----
 .../frontend/base_flow_graph_builder.cc       | 19 ++++++++++++++-----
 tests/ffi/vmspecific_static_checks_test.dart  |  9 +++++++++
 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/pkg/vm/lib/transformations/ffi/common.dart b/pkg/vm/lib/transformations/ffi/common.dart
index d3bf8d0052e..391beb3a987 100644
--- a/pkg/vm/lib/transformations/ffi/common.dart
+++ b/pkg/vm/lib/transformations/ffi/common.dart
@@ -1259,13 +1259,19 @@ class FfiTransformer extends Transformer {
     if (env.isSubtypeOf(correspondingDartType, dartType,
         SubtypeCheckMode.ignoringNullabilities)) {
       // If subtype, manually check the return type is not void.
-      if (dartType is! FunctionType || correspondingDartType is! FunctionType) {
-        return;
-      } else if ((dartType.returnType is VoidType) ==
-          (correspondingDartType.returnType is VoidType)) {
+      if (correspondingDartType is FunctionType) {
+        if (dartType is FunctionType) {
+          if ((dartType.returnType is VoidType) ==
+              (correspondingDartType.returnType is VoidType)) {
+            return;
+          }
+          // One of the return types is void, the other isn't, report error.
+        } else {
+          // One is a function type, the other isn't, report error.
+        }
+      } else {
         return;
       }
-      // One of the return types is void, the other isn't, report error.
     }
     diagnosticReporter.report(
         templateFfiTypeMismatch.withArguments(dartType, correspondingDartType,
diff --git a/runtime/vm/compiler/frontend/base_flow_graph_builder.cc b/runtime/vm/compiler/frontend/base_flow_graph_builder.cc
index db26aaaaaea..59c2d6835ef 100644
--- a/runtime/vm/compiler/frontend/base_flow_graph_builder.cc
+++ b/runtime/vm/compiler/frontend/base_flow_graph_builder.cc
@@ -1028,13 +1028,22 @@ Fragment BaseFlowGraphBuilder::Box(Representation from) {
 Fragment BaseFlowGraphBuilder::BuildFfiAsFunctionInternalCall(
     const TypeArguments& signatures,
     bool is_leaf) {
-  ASSERT(signatures.IsInstantiated());
   ASSERT(signatures.Length() == 2);
+  const auto& sig0 = AbstractType::Handle(signatures.TypeAt(0));
+  const auto& sig1 = AbstractType::Handle(signatures.TypeAt(1));
 
-  const auto& dart_type =
-      FunctionType::Cast(AbstractType::Handle(signatures.TypeAt(0)));
-  const auto& native_type =
-      FunctionType::Cast(AbstractType::Handle(signatures.TypeAt(1)));
+  if (!signatures.IsInstantiated() || !sig0.IsFunctionType() ||
+      !sig1.IsFunctionType()) {
+    const auto& msg = String::Handle(String::NewFormatted(
+        "Invalid type arguments passed to dart:ffi _asFunctionInternal: %s",
+        String::Handle(signatures.UserVisibleName()).ToCString()));
+    const auto& language_error =
+        Error::Handle(LanguageError::New(msg, Report::kError, Heap::kOld));
+    Report::LongJump(language_error);
+  }
+
+  const auto& dart_type = FunctionType::Cast(sig0);
+  const auto& native_type = FunctionType::Cast(sig1);
 
   // AbiSpecificTypes can have an incomplete mapping.
   const char* error = nullptr;
diff --git a/tests/ffi/vmspecific_static_checks_test.dart b/tests/ffi/vmspecific_static_checks_test.dart
index e65ece1be01..95543938d17 100644
--- a/tests/ffi/vmspecific_static_checks_test.dart
+++ b/tests/ffi/vmspecific_static_checks_test.dart
@@ -279,6 +279,15 @@ void testAsFunctionTypeMismatch() {
   // [cfe] Expected type 'int Function(int, int)' to be 'int Function(int)', which is the Dart type corresponding to 'NativeFunction<Int8 Function(Int8)>'.
 }
 
+void testFunctionNotFunctionType() {
+  Pointer<NativeFunction<Int8UnOp>> p = Pointer.fromAddress(1337);
+  Function f = p.asFunction();
+  //           ^^^^^^^^^^^^^^
+  // [analyzer] COMPILE_TIME_ERROR.MUST_BE_A_SUBTYPE
+  //             ^
+  // [cfe] Expected type 'Function' to be 'int Function(int)', which is the Dart type corresponding to 'NativeFunction<Int8 Function(Int8)>'.
+}
+
 typedef NativeDoubleUnOp = Double Function(Double);
 typedef DoubleUnOp = double Function(double);