[VM/Runtime] 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: I4a3cb28370d27306c795c1914aeb5c18a1d85e2b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/210761
Reviewed-by: Alexander Aprelev <aam@google.com>
Commit-Queue: Siva Annamalai <asiva@google.com>
diff --git a/runtime/bin/dartutils.cc b/runtime/bin/dartutils.cc
index 9f60f2a..61bef4b 100644
--- a/runtime/bin/dartutils.cc
+++ b/runtime/bin/dartutils.cc
@@ -931,6 +931,16 @@
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
diff --git a/runtime/bin/dartutils.h b/runtime/bin/dartutils.h
index 7ff1580..4eadf084 100644
--- a/runtime/bin/dartutils.h
+++ b/runtime/bin/dartutils.h
@@ -351,6 +351,9 @@
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 @@
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(); }
diff --git a/runtime/bin/file.cc b/runtime/bin/file.cc
index 8e377a1..fad12cf 100644
--- a/runtime/bin/file.cc
+++ b/runtime/bin/file.cc
@@ -923,8 +923,8 @@
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) {
diff --git a/runtime/include/dart_native_api.h b/runtime/include/dart_native_api.h
index ac183b8..f99fff1 100644
--- a/runtime/include/dart_native_api.h
+++ b/runtime/include/dart_native_api.h
@@ -45,6 +45,7 @@
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 @@
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
diff --git a/runtime/vm/class_id.h b/runtime/vm/class_id.h
index 5ca709a..e7f7cbc 100644
--- a/runtime/vm/class_id.h
+++ b/runtime/vm/class_id.h
@@ -203,6 +203,10 @@
// 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 @@
// 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;
}
diff --git a/runtime/vm/message_snapshot.cc b/runtime/vm/message_snapshot.cc
index e65fc1c..ef3fba1 100644
--- a/runtime/vm/message_snapshot.cc
+++ b/runtime/vm/message_snapshot.cc
@@ -1818,6 +1818,67 @@
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);
+
+ intptr_t ptr = data->value.as_native_pointer.ptr;
+ s->WriteUnsigned(ptr);
+
+ s->finalizable_data()->Put(
+ data->value.as_native_pointer.size, nullptr,
+ 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++) {
+ intptr_t ptr = d->ReadUnsigned();
+ d->finalizable_data()->Take();
+ d->AssignRef(Integer::New(ptr));
+ }
+ }
+
+ void ReadNodesApi(ApiMessageDeserializer* d) { UNREACHABLE(); }
+
+ private:
+ const intptr_t cid_;
+};
+
class TypedDataViewMessageSerializationCluster
: public MessageSerializationCluster {
public:
@@ -3189,6 +3250,9 @@
case Dart_CObject_kCapability:
cid = kCapabilityCid;
break;
+ case Dart_CObject_kNativePointer:
+ cid = kNativePointer;
+ break;
default:
return Fail("invalid Dart_CObject type");
}
@@ -3242,6 +3306,8 @@
}
switch (cid) {
+ case kNativePointer:
+ return new (Z) NativePointerMessageSerializationCluster(Z);
case kClassCid:
return new (Z) ClassMessageSerializationCluster();
case kTypeArgumentsCid:
@@ -3326,6 +3392,9 @@
}
switch (cid) {
+ case kNativePointer:
+ ASSERT(!is_canonical);
+ return new (Z) NativePointerMessageDeserializationCluster();
case kClassCid:
ASSERT(!is_canonical);
return new (Z) ClassMessageDeserializationCluster();
diff --git a/tests/standalone/io/file_leak_test.dart b/tests/standalone/io/file_leak_test.dart
new file mode 100644
index 0000000..6c8ddf1
--- /dev/null
+++ b/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();
+}
diff --git a/tests/standalone_2/io/file_leak_test.dart b/tests/standalone_2/io/file_leak_test.dart
new file mode 100644
index 0000000..9bac244
--- /dev/null
+++ b/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();
+}
