From 04c984ce3f93b68b8d72fbd686e2fe7656477bb1 Mon Sep 17 00:00:00 2001
From: Daco Harkes <dacoharkes@google.com>
Date: Thu, 27 Jun 2019 14:26:33 +0000
Subject: [PATCH] [vm/ffi] Make overflow checks consistent

This CL changes the semantics of Pointer.allocate() to not do any range or overflow checks. This is consistent with the truncating behavior that the rest of the FFI has.

Fixes: https://github.com/dart-lang/sdk/issues/37250

Change-Id: Icc2b53e229cd6a2faae99c833ea5df372eb35b74
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/107503
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Samir Jindel <sjindel@google.com>
---
 runtime/lib/ffi.cc       | 19 +++----------------
 tests/ffi/data_test.dart | 20 +++++++++++++++++++-
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/runtime/lib/ffi.cc b/runtime/lib/ffi.cc
index e838a399e70..837bbd0a11b 100644
--- a/runtime/lib/ffi.cc
+++ b/runtime/lib/ffi.cc
@@ -169,18 +169,6 @@ static void CheckDartAndCTypeCorrespond(const AbstractType& native_type,
 
 // The following functions are runtime checks on arguments.
 
-// Note that expected_from and expected_to are inclusive.
-static void CheckRange(const Integer& argument_value,
-                       intptr_t expected_from,
-                       intptr_t expected_to,
-                       const char* argument_name) {
-  int64_t value = argument_value.AsInt64Value();
-  if (value < expected_from || expected_to < value) {
-    Exceptions::ThrowRangeError(argument_name, argument_value, expected_from,
-                                expected_to);
-  }
-}
-
 static const Pointer& AsPointer(const Instance& instance) {
   if (!instance.IsPointer()) {
     const String& error = String::Handle(String::NewFormatted(
@@ -221,11 +209,10 @@ DEFINE_NATIVE_ENTRY(Ffi_allocate, 1, 1) {
   GET_NON_NULL_NATIVE_ARGUMENT(Integer, argCount, arguments->NativeArgAt(0));
   int64_t count = argCount.AsInt64Value();
   classid_t type_cid = type_arg.type_class_id();
-  int64_t max_count = INTPTR_MAX / compiler::ffi::ElementSizeInBytes(type_cid);
-  CheckRange(argCount, 1, max_count, "count");
 
-  size_t size = compiler::ffi::ElementSizeInBytes(type_cid) * count;
-  uint64_t memory = reinterpret_cast<uint64_t>(malloc(size));
+  size_t size = compiler::ffi::ElementSizeInBytes(type_cid) *
+                count;  // Truncates overflow.
+  size_t memory = reinterpret_cast<size_t>(malloc(size));
   if (memory == 0) {
     const String& error = String::Handle(String::NewFormatted(
         "allocating (%" Pd ") bytes of memory failed", size));
diff --git a/tests/ffi/data_test.dart b/tests/ffi/data_test.dart
index bd3530aceeb..952fc5906d4 100644
--- a/tests/ffi/data_test.dart
+++ b/tests/ffi/data_test.dart
@@ -93,7 +93,25 @@ void testPointerPointerArithmeticSizes() {
 }
 
 void testPointerAllocateNonPositive() {
-  Expect.throws(() => ffi.allocate<ffi.Int8>(count: 0));
+  // > If size is 0, either a null pointer or a unique pointer that can be
+  // > successfully passed to free() shall be returned.
+  // http://pubs.opengroup.org/onlinepubs/009695399/functions/malloc.html
+  //
+  // Null pointer throws a Dart exception.
+  bool returnedNullPointer = false;
+  ffi.Pointer<ffi.Int8> p;
+  try {
+    p = ffi.allocate<ffi.Int8>(count: 0);
+  } on Exception {
+    returnedNullPointer = true;
+  }
+  if (!returnedNullPointer) {
+    p.free();
+  }
+
+  // Passing in -1 will be converted into an unsigned integer. So, it will try
+  // to allocate SIZE_MAX - 1 + 1 bytes. This will fail as it is the max amount
+  // of addressable memory on the system.
   Expect.throws(() => ffi.allocate<ffi.Int8>(count: -1));
 }