[VM/Runtime] Reland : Fix 'File' object leak in async file open operation

The 'File' object created in the async file open method is leaked
if the operation is not completed when the isolate shuts down
with an unhandled exception.
This change adds a finalizable state for the 'File' object so that
the message deletion that happens when ports are closed could run
the callback to clean up the 'File' object.


TEST=new tests added

Change-Id: I64c18a7905261b0fc7bf9f220086791478232d0d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/211340
Reviewed-by: Alexander Aprelev <aam@google.com>
Commit-Queue: Siva Annamalai <asiva@google.com>

runtime/bin/dartutils.cc

@@ -931,6 +931,16 @@ Dart_CObject* CObject::NewExternalUint8Array(intptr_t length,
   return cobject;
 }
 
+Dart_CObject* CObject::NewNativePointer(intptr_t ptr,
+                                        intptr_t size,
+                                        Dart_HandleFinalizer callback) {
+  Dart_CObject* cobject = New(Dart_CObject_kNativePointer);
+  cobject->value.as_native_pointer.ptr = ptr;
+  cobject->value.as_native_pointer.size = size;
+  cobject->value.as_native_pointer.callback = callback;
+  return cobject;
+}
+
 Dart_CObject* CObject::NewIOBuffer(int64_t length) {
   // Make sure that we do not have an integer overflow here. Actual check
   // against max elements will be done at the time of writing, as the constant

runtime/bin/dartutils.h

@@ -351,6 +351,9 @@ class CObject {
                                              uint8_t* data,
                                              void* peer,
                                              Dart_HandleFinalizer callback);
+  static Dart_CObject* NewNativePointer(intptr_t ptr,
+                                        intptr_t size,
+                                        Dart_HandleFinalizer callback);
 
   static Dart_CObject* NewIOBuffer(int64_t length);
   static void ShrinkIOBuffer(Dart_CObject* cobject, int64_t new_length);
@@ -579,6 +582,20 @@ class CObjectExternalUint8Array : public CObject {
   DISALLOW_COPY_AND_ASSIGN(CObjectExternalUint8Array);
 };
 
+class CObjectNativePointer : public CObject {
+ public:
+  DECLARE_COBJECT_CONSTRUCTORS(NativePointer)
+
+  intptr_t Ptr() const { return cobject_->value.as_native_pointer.ptr; }
+  intptr_t Size() const { return cobject_->value.as_native_pointer.size; }
+  Dart_HandleFinalizer Callback() const {
+    return cobject_->value.as_native_pointer.callback;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CObjectNativePointer);
+};
+
 class ScopedBlockingCall {
  public:
   ScopedBlockingCall() { Dart_ThreadDisableProfiling(); }

runtime/bin/file.cc

@@ -923,8 +923,8 @@ CObject* File::OpenRequest(const CObjectArray& request) {
   if (file == NULL) {
     return CObject::NewOSError();
   }
-  return new CObjectIntptr(
-      CObject::NewIntptr(reinterpret_cast<intptr_t>(file)));
+  return new CObjectNativePointer(CObject::NewNativePointer(
+      reinterpret_cast<intptr_t>(file), sizeof(*file), ReleaseFile));
 }
 
 CObject* File::DeleteRequest(const CObjectArray& request) {

runtime/include/dart_native_api.h

@@ -45,6 +45,7 @@ typedef enum {
   Dart_CObject_kExternalTypedData,
   Dart_CObject_kSendPort,
   Dart_CObject_kCapability,
+  Dart_CObject_kNativePointer,
   Dart_CObject_kUnsupported,
   Dart_CObject_kNumberOfTypes
 } Dart_CObject_Type;
@@ -80,6 +81,11 @@ typedef struct _Dart_CObject {
       void* peer;
       Dart_HandleFinalizer callback;
     } as_external_typed_data;
+    struct {
+      intptr_t ptr;
+      intptr_t size;
+      Dart_HandleFinalizer callback;
+    } as_native_pointer;
   } value;
 } Dart_CObject;
 // This struct is versioned by DART_API_DL_MAJOR_VERSION, bump the version when

runtime/vm/class_id.h

@@ -203,6 +203,10 @@ enum ClassId : intptr_t {
   // Illegal class id.
   kIllegalCid = 0,
 
+  // Pseudo class id for native pointers, the heap should never see an
+  // object with this class id.
+  kNativePointer,
+
   // The following entries describes classes for pseudo-objects in the heap
   // that should never be reachable from live objects. Free list elements
   // maintain the free list for old space, and forwarding corpses are used to
@@ -281,10 +285,11 @@ COMPILE_ASSERT(kInstanceCid == kLastInternalOnlyCid + 1);
 // and should not be exposed directly to user code.
 inline bool IsInternalOnlyClassId(intptr_t index) {
   // Fix the condition below if these become non-contiguous.
-  COMPILE_ASSERT(kIllegalCid + 1 == kFreeListElement &&
-                 kIllegalCid + 2 == kForwardingCorpse &&
-                 kIllegalCid + 3 == kObjectCid &&
-                 kIllegalCid + 4 == kFirstInternalOnlyCid);
+  COMPILE_ASSERT(kIllegalCid + 1 == kNativePointer &&
+                 kIllegalCid + 2 == kFreeListElement &&
+                 kIllegalCid + 3 == kForwardingCorpse &&
+                 kIllegalCid + 4 == kObjectCid &&
+                 kIllegalCid + 5 == kFirstInternalOnlyCid);
   return index <= kLastInternalOnlyCid;
 }
 

runtime/vm/message_snapshot.cc

@@ -1818,6 +1818,65 @@ class ExternalTypedDataMessageDeserializationCluster
   const intptr_t cid_;
 };
 
+class NativePointerMessageSerializationCluster
+    : public MessageSerializationCluster {
+ public:
+  explicit NativePointerMessageSerializationCluster(Zone* zone)
+      : MessageSerializationCluster("NativePointer",
+                                    MessagePhase::kNonCanonicalInstances,
+                                    kNativePointer),
+        objects_(zone, 0) {}
+  ~NativePointerMessageSerializationCluster() {}
+
+  void Trace(MessageSerializer* s, Object* object) { UNREACHABLE(); }
+
+  void WriteNodes(MessageSerializer* s) { UNREACHABLE(); }
+
+  void TraceApi(ApiMessageSerializer* s, Dart_CObject* object) {
+    objects_.Add(object);
+  }
+
+  void WriteNodesApi(ApiMessageSerializer* s) {
+    intptr_t count = objects_.length();
+    s->WriteUnsigned(count);
+    for (intptr_t i = 0; i < count; i++) {
+      Dart_CObject* data = objects_[i];
+      s->AssignRef(data);
+
+      s->finalizable_data()->Put(
+          data->value.as_native_pointer.size,
+          reinterpret_cast<void*>(data->value.as_native_pointer.ptr),
+          reinterpret_cast<void*>(data->value.as_native_pointer.ptr),
+          data->value.as_native_pointer.callback);
+    }
+  }
+
+ private:
+  GrowableArray<Dart_CObject*> objects_;
+};
+
+class NativePointerMessageDeserializationCluster
+    : public MessageDeserializationCluster {
+ public:
+  NativePointerMessageDeserializationCluster()
+      : MessageDeserializationCluster("NativePointer"), cid_(kNativePointer) {}
+  ~NativePointerMessageDeserializationCluster() {}
+
+  void ReadNodes(MessageDeserializer* d) {
+    intptr_t count = d->ReadUnsigned();
+    for (intptr_t i = 0; i < count; i++) {
+      FinalizableData finalizable_data = d->finalizable_data()->Take();
+      intptr_t ptr = reinterpret_cast<intptr_t>(finalizable_data.data);
+      d->AssignRef(Integer::New(ptr));
+    }
+  }
+
+  void ReadNodesApi(ApiMessageDeserializer* d) { UNREACHABLE(); }
+
+ private:
+  const intptr_t cid_;
+};
+
 class TypedDataViewMessageSerializationCluster
     : public MessageSerializationCluster {
  public:
@@ -3189,6 +3248,9 @@ bool ApiMessageSerializer::Trace(Dart_CObject* object) {
     case Dart_CObject_kCapability:
       cid = kCapabilityCid;
       break;
+    case Dart_CObject_kNativePointer:
+      cid = kNativePointer;
+      break;
     default:
       return Fail("invalid Dart_CObject type");
   }
@@ -3242,6 +3304,8 @@ MessageSerializationCluster* BaseSerializer::NewClusterForClass(
   }
 
   switch (cid) {
+    case kNativePointer:
+      return new (Z) NativePointerMessageSerializationCluster(Z);
     case kClassCid:
       return new (Z) ClassMessageSerializationCluster();
     case kTypeArgumentsCid:
@@ -3326,6 +3390,9 @@ MessageDeserializationCluster* BaseDeserializer::ReadCluster() {
   }
 
   switch (cid) {
+    case kNativePointer:
+      ASSERT(!is_canonical);
+      return new (Z) NativePointerMessageDeserializationCluster();
     case kClassCid:
       ASSERT(!is_canonical);
       return new (Z) ClassMessageDeserializationCluster();

tests/standalone/io/file_leak_test.dart

@@ -0,0 +1,139 @@
+// Copyright (c) 2021, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+//
+// Dart test program for testing file I/O.
+
+// OtherResources=fixed_length_file
+// OtherResources=read_as_text.dat
+
+import 'dart:async';
+import 'dart:convert';
+import 'dart:collection';
+import 'dart:io';
+
+import "package:async_helper/async_helper.dart";
+import "package:expect/expect.dart";
+import "package:path/path.dart";
+
+class FileTest {
+  static Directory? tempDirectory;
+  static int numLiveAsyncTests = 0;
+
+  static void asyncTestStarted() {
+    asyncStart();
+    ++numLiveAsyncTests;
+  }
+
+  static void asyncTestDone(String name) {
+    asyncEnd();
+    --numLiveAsyncTests;
+    if (numLiveAsyncTests == 0) {
+      deleteTempDirectory();
+    }
+  }
+
+  static void createTempDirectory(Function doNext) {
+    Directory.systemTemp.createTemp('dart_file').then((temp) {
+      tempDirectory = temp;
+      doNext();
+    });
+  }
+
+  static void deleteTempDirectory() {
+    tempDirectory!.deleteSync(recursive: true);
+  }
+
+  static testReadInto() async {
+    asyncTestStarted();
+    File file = new File(tempDirectory!.path + "/out_read_into");
+
+    var openedFile = await file.open(mode: FileMode.write);
+    await openedFile.writeFrom(const [1, 2, 3]);
+
+    await openedFile.setPosition(0);
+    var list = <int>[1, 2, 3];
+    Expect.equals(3, await openedFile.readInto(list));
+    Expect.listEquals([1, 2, 3], list);
+
+    read(start, end, length, expected) async {
+      var list = <int>[1, 2, 3];
+      await openedFile.setPosition(0);
+      Expect.equals(length, await openedFile.readInto(list, start, end));
+      Expect.listEquals(expected, list);
+      return list;
+    }
+
+    await read(0, 3, 3, [1, 2, 3]);
+    await read(0, 2, 2, [1, 2, null]);
+    await read(1, 2, 1, [null, 1, null]);
+    await read(1, 3, 2, [null, 1, 2]);
+    await read(2, 3, 1, [null, null, 1]);
+    await read(0, 0, 0, [null, null, null]);
+
+    await openedFile.close();
+
+    asyncTestDone("testReadInto");
+  }
+
+  static void testReadAsText() {
+    asyncTestStarted();
+    var name = getFilename("fixed_length_file");
+    var f = new File(name);
+    f.readAsString(encoding: utf8).then((text) {
+      Expect.isTrue(text.endsWith("42 bytes."));
+      Expect.equals(42, text.length);
+      var name = getFilename("read_as_text.dat");
+      var f = new File(name);
+      f.readAsString(encoding: utf8).then((text) {
+        Expect.equals(6, text.length);
+        var expected = [955, 120, 46, 32, 120, 10];
+        Expect.listEquals(expected, text.codeUnits);
+        f.readAsString(encoding: latin1).then((text) {
+          Expect.equals(7, text.length);
+          var expected = [206, 187, 120, 46, 32, 120, 10];
+          Expect.listEquals(expected, text.codeUnits);
+          var readAsStringFuture = f.readAsString(encoding: ascii);
+          readAsStringFuture.then((text) {
+            Expect.fail("Non-ascii char should cause error");
+          }).catchError((e) {
+            asyncTestDone("testReadAsText");
+          });
+        });
+      });
+    });
+  }
+
+  static String getFilename(String path) {
+    return Platform.script.resolve(path).toFilePath();
+  }
+
+  // Main test entrypoint.
+  // This test results in an unhandled exception in the isolate while
+  // some async file IO operations are still pending. The unhandled
+  // exception results in the 'File' object being leaked, the error
+  // only shows up in the ASAN bots which detect the leak.
+  static testMain() {
+    asyncStart();
+    var outerZone = Zone.current;
+    var firstZone = Zone.current.fork(specification: ZoneSpecification(
+        handleUncaughtError: (self, parent, zone, error, stacktrace) {
+      asyncEnd();
+      print("unittest-suite-success"); // For the test harness.
+      exit(0);
+    }));
+    firstZone.run(() async {
+      Expect.identical(firstZone, Zone.current);
+      createTempDirectory(() {
+        testReadAsText();
+        testReadInto();
+        Expect.equals(1, 0); // Should not execute this.
+        asyncEnd();
+      });
+    });
+  }
+}
+
+main() {
+  FileTest.testMain();
+}

tests/standalone_2/io/file_leak_test.dart

@@ -0,0 +1,141 @@
+// Copyright (c) 2021, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+//
+// Dart test program for testing file I/O.
+
+// @dart = 2.9
+
+// OtherResources=fixed_length_file
+// OtherResources=read_as_text.dat
+
+import 'dart:async';
+import 'dart:convert';
+import 'dart:collection';
+import 'dart:io';
+
+import "package:async_helper/async_helper.dart";
+import "package:expect/expect.dart";
+import "package:path/path.dart";
+
+class FileTest {
+  static Directory tempDirectory;
+  static int numLiveAsyncTests = 0;
+
+  static void asyncTestStarted() {
+    asyncStart();
+    ++numLiveAsyncTests;
+  }
+
+  static void asyncTestDone(String name) {
+    asyncEnd();
+    --numLiveAsyncTests;
+    if (numLiveAsyncTests == 0) {
+      deleteTempDirectory();
+    }
+  }
+
+  static void createTempDirectory(Function doNext) {
+    Directory.systemTemp.createTemp('dart_file').then((temp) {
+      tempDirectory = temp;
+      doNext();
+    });
+  }
+
+  static void deleteTempDirectory() {
+    tempDirectory.deleteSync(recursive: true);
+  }
+
+  static testReadInto() async {
+    asyncTestStarted();
+    File file = new File(tempDirectory.path + "/out_read_into");
+
+    var openedFile = await file.open(mode: FileMode.write);
+    await openedFile.writeFrom(const [1, 2, 3]);
+
+    await openedFile.setPosition(0);
+    var list = [null, null, null];
+    Expect.equals(3, await openedFile.readInto(list));
+    Expect.listEquals([1, 2, 3], list);
+
+    read(start, end, length, expected) async {
+      var list = [null, null, null];
+      await openedFile.setPosition(0);
+      Expect.equals(length, await openedFile.readInto(list, start, end));
+      Expect.listEquals(expected, list);
+      return list;
+    }
+
+    await read(0, 3, 3, [1, 2, 3]);
+    await read(0, 2, 2, [1, 2, null]);
+    await read(1, 2, 1, [null, 1, null]);
+    await read(1, 3, 2, [null, 1, 2]);
+    await read(2, 3, 1, [null, null, 1]);
+    await read(0, 0, 0, [null, null, null]);
+
+    await openedFile.close();
+
+    asyncTestDone("testReadInto");
+  }
+
+  static void testReadAsText() {
+    asyncTestStarted();
+    var name = getFilename("fixed_length_file");
+    var f = new File(name);
+    f.readAsString(encoding: utf8).then((text) {
+      Expect.isTrue(text.endsWith("42 bytes."));
+      Expect.equals(42, text.length);
+      var name = getFilename("read_as_text.dat");
+      var f = new File(name);
+      f.readAsString(encoding: utf8).then((text) {
+        Expect.equals(6, text.length);
+        var expected = [955, 120, 46, 32, 120, 10];
+        Expect.listEquals(expected, text.codeUnits);
+        f.readAsString(encoding: latin1).then((text) {
+          Expect.equals(7, text.length);
+          var expected = [206, 187, 120, 46, 32, 120, 10];
+          Expect.listEquals(expected, text.codeUnits);
+          var readAsStringFuture = f.readAsString(encoding: ascii);
+          readAsStringFuture.then((text) {
+            Expect.fail("Non-ascii char should cause error");
+          }).catchError((e) {
+            asyncTestDone("testReadAsText");
+          });
+        });
+      });
+    });
+  }
+
+  static String getFilename(String path) {
+    return Platform.script.resolve(path).toFilePath();
+  }
+
+  // Main test entrypoint.
+  // This test results in an unhandled exception in the isolate while
+  // some async file IO operations are still pending. The unhandled
+  // exception results in the 'File' object being leaked, the error
+  // only shows up in the ASAN bots which detect the leak.
+  static testMain() {
+    asyncStart();
+    var outerZone = Zone.current;
+    var firstZone = Zone.current.fork(specification: ZoneSpecification(
+        handleUncaughtError: (self, parent, zone, error, stacktrace) {
+      asyncEnd();
+      print("unittest-suite-success"); // For the test harness.
+      exit(0);
+    }));
+    firstZone.run(() async {
+      Expect.identical(firstZone, Zone.current);
+      createTempDirectory(() {
+        testReadAsText();
+        testReadInto();
+        Expect.equals(1, 0); // Should not execute this.
+        asyncEnd();
+      });
+    });
+  }
+}
+
+main() {
+  FileTest.testMain();
+}