From bc387837c4f1162d23a1434a8715ec4175fed9be Mon Sep 17 00:00:00 2001 From: asiva Date: Fri, 27 Aug 2021 17:45:17 +0000 Subject: [PATCH] [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 Commit-Queue: Siva Annamalai --- runtime/bin/dartutils.cc | 10 ++ runtime/bin/dartutils.h | 17 +++ runtime/bin/file.cc | 4 +- runtime/include/dart_native_api.h | 6 + runtime/vm/class_id.h | 13 +- runtime/vm/message_snapshot.cc | 67 ++++++++++ tests/standalone/io/file_leak_test.dart | 139 +++++++++++++++++++++ tests/standalone_2/io/file_leak_test.dart | 141 ++++++++++++++++++++++ 8 files changed, 391 insertions(+), 6 deletions(-) create mode 100644 tests/standalone/io/file_leak_test.dart create mode 100644 tests/standalone_2/io/file_leak_test.dart diff --git a/runtime/bin/dartutils.cc b/runtime/bin/dartutils.cc index 9f60f2a4b6c..61bef4b55ad 100644 --- a/runtime/bin/dartutils.cc +++ b/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 diff --git a/runtime/bin/dartutils.h b/runtime/bin/dartutils.h index 7ff158006eb..4eadf084bb1 100644 --- a/runtime/bin/dartutils.h +++ b/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(); } diff --git a/runtime/bin/file.cc b/runtime/bin/file.cc index 8e377a18513..fad12cf4962 100644 --- a/runtime/bin/file.cc +++ b/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(file))); + return new CObjectNativePointer(CObject::NewNativePointer( + reinterpret_cast(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 ac183b82c5e..f99fff1150a 100644 --- a/runtime/include/dart_native_api.h +++ b/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 diff --git a/runtime/vm/class_id.h b/runtime/vm/class_id.h index 5ca709a90b3..e7f7cbc1eb0 100644 --- a/runtime/vm/class_id.h +++ b/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; } diff --git a/runtime/vm/message_snapshot.cc b/runtime/vm/message_snapshot.cc index e65fc1c4e04..6c69e47fafb 100644 --- a/runtime/vm/message_snapshot.cc +++ b/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(data->value.as_native_pointer.ptr), + reinterpret_cast(data->value.as_native_pointer.ptr), + data->value.as_native_pointer.callback); + } + } + + private: + GrowableArray 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(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(); diff --git a/tests/standalone/io/file_leak_test.dart b/tests/standalone/io/file_leak_test.dart new file mode 100644 index 00000000000..6c8ddf1022f --- /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 = [1, 2, 3]; + Expect.equals(3, await openedFile.readInto(list)); + Expect.listEquals([1, 2, 3], list); + + read(start, end, length, expected) async { + var list = [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 00000000000..9bac244122c --- /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(); +}