Fixes leak of native File objects.

runtime/bin/file.cc

 // Copyright (c) 2013, 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.
 
 #if !defined(DART_IO_DISABLED)
 
 #include "bin/file.h"
 
 #include "bin/builtin.h"
 #include "bin/dartutils.h"
 #include "bin/embedded_dart_io.h"
 #include "bin/io_buffer.h"
 #include "bin/utils.h"
 
 #include "include/dart_api.h"
 #include "include/dart_tools_api.h"
 
 namespace dart {
 namespace bin {
 
+static const int kFileNativeFieldIndex = 0;
 static const int kMSPerSecond = 1000;
 
 // The file pointer has been passed into Dart as an intptr_t and it is safe
 // to pull it out of Dart as a 64-bit integer, cast it to an intptr_t and
 // from there to a File pointer.
-static File* GetFilePointer(Dart_Handle handle) {
-  intptr_t value = DartUtils::GetIntptrValue(handle);
-  return reinterpret_cast<File*>(value);
+static File* GetFile(Dart_NativeArguments args) {
+  File* file;
+  Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
+  ASSERT(Dart_IsInstance(dart_this));
+  ThrowIfError(Dart_GetNativeInstanceField(
+      dart_this,
+      kFileNativeFieldIndex,
+      reinterpret_cast<intptr_t*>(&file)));
+  return file;
 }
 
 
+static void SetFile(Dart_Handle dart_this, intptr_t file_pointer) {
+  Dart_Handle result = Dart_SetNativeInstanceField(
+      dart_this,
+      kFileNativeFieldIndex,
+      file_pointer);
+  if (Dart_IsError(result)) {
+    Log::PrintErr("SetNativeInstanceField in SetFile() failed\n");
+    Dart_PropagateError(result);
+  }
+}
+
+
+void FUNCTION_NAME(File_GetPointer)(Dart_NativeArguments args) {
+  File* file = GetFile(args);
+  // If the file is already closed, GetFile() will return NULL.
+  if (file != NULL) {
+    // Increment file's reference count. File_GetPointer() should only be called
+    // when we are about to send the File* to the IO Service.
+    file->Retain();
+  }
+  intptr_t file_pointer = reinterpret_cast<intptr_t>(file);
+  Dart_SetReturnValue(args, Dart_NewInteger(file_pointer));
+}
+
+
+static void ReleaseFile(void* isolate_callback_data,
+                        Dart_WeakPersistentHandle handle,
+                        void* peer) {
+  File* file = reinterpret_cast<File*>(peer);
+  file->Release();
+}
+
+
+void FUNCTION_NAME(File_SetPointer)(Dart_NativeArguments args) {
+  Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
+  intptr_t file_pointer =
+      DartUtils::GetIntptrValue(Dart_GetNativeArgument(args, 1));
+  File* file = reinterpret_cast<File*>(file_pointer);
+  Dart_WeakPersistentHandle handle = Dart_NewWeakPersistentHandle(
+      dart_this, reinterpret_cast<void*>(file), sizeof(*file), ReleaseFile);
+  file->SetWeakHandle(handle);
+  SetFile(dart_this, file_pointer);
+}
+
+
 void FUNCTION_NAME(File_Open)(Dart_NativeArguments args) {
   const char* filename =
       DartUtils::GetStringValue(Dart_GetNativeArgument(args, 0));
   int64_t mode = DartUtils::GetIntegerValue(Dart_GetNativeArgument(args, 1));
   File::DartFileOpenMode dart_file_mode =
       static_cast<File::DartFileOpenMode>(mode);
   File::FileOpenMode file_mode = File::DartModeToFileMode(dart_file_mode);
   // Check that the file exists before opening it only for
   // reading. This is to prevent the opening of directories as
   // files. Directories can be opened for reading using the posix
   // 'open' call.
-  File* file = NULL;
-  file = File::ScopedOpen(filename, file_mode);
+  File* file = File::ScopedOpen(filename, file_mode);
   if (file != NULL) {
     Dart_SetReturnValue(args,
                         Dart_NewInteger(reinterpret_cast<intptr_t>(file)));
   } else {
     Dart_SetReturnValue(args, DartUtils::NewDartOSError());
   }
 }
 
 
 void FUNCTION_NAME(File_Exists)(Dart_NativeArguments args) {
   const char* filename =
       DartUtils::GetStringValue(Dart_GetNativeArgument(args, 0));
   bool exists = File::Exists(filename);
   Dart_SetReturnValue(args, Dart_NewBoolean(exists));
 }
 
 
 void FUNCTION_NAME(File_Close)(Dart_NativeArguments args) {
-  File* file = GetFilePointer(Dart_GetNativeArgument(args, 0));
+  File* file = GetFile(args);
   ASSERT(file != NULL);
-  delete file;
+  file->DeleteWeakHandle(Dart_CurrentIsolate());
+  file->Release();
+
+  // NULL-out the now potentially dangling pointer.
+  Dart_Handle dart_this = Dart_GetNativeArgument(args, 0);

[Ivan Posva, 2016/04/20 05:31:06]

Can you rely on the Dart code to do the disassociation with the native object
instead?

[zra, 2016/04/20 17:07:52]

The native object is associated by extending NativeFieldWrapperClass1, so AFAIU
it isn't possible to touch the field in Dart code.

+  SetFile(dart_this, 0);
   Dart_SetReturnValue(args, Dart_NewInteger(0));
 }
 
 
-void FUNCTION_NAME(File_GetFD)(Dart_NativeArguments args) {
-  File* file = GetFilePointer(Dart_GetNativeArgument(args, 0));
-  ASSERT(file != NULL);
-  Dart_SetReturnValue(args, Dart_NewInteger(file->GetFD()));
-}
-
-
 void FUNCTION_NAME(File_ReadByte)(Dart_NativeArguments args) {
-  File* file = GetFilePointer(Dart_GetNativeArgument(args, 0));
+  File* file = GetFile(args);
   ASSERT(file != NULL);
   uint8_t buffer;
   int64_t bytes_read = file->Read(reinterpret_cast<void*>(&buffer), 1);
   if (bytes_read == 1) {
     Dart_SetReturnValue(args, Dart_NewInteger(buffer));
   } else if (bytes_read == 0) {
     Dart_SetReturnValue(args, Dart_NewInteger(-1));
   } else {
     Dart_SetReturnValue(args, DartUtils::NewDartOSError());
   }
 }
 
 
 void FUNCTION_NAME(File_WriteByte)(Dart_NativeArguments args) {
-  File* file = GetFilePointer(Dart_GetNativeArgument(args, 0));
+  File* file = GetFile(args);
   ASSERT(file != NULL);
   int64_t byte = 0;
   if (DartUtils::GetInt64Value(Dart_GetNativeArgument(args, 1), &byte)) {
     uint8_t buffer = static_cast<uint8_t>(byte & 0xff);
     bool success = file->WriteFully(reinterpret_cast<void*>(&buffer), 1);
     if (success) {
       Dart_SetReturnValue(args, Dart_NewInteger(1));
     } else {
       Dart_SetReturnValue(args, DartUtils::NewDartOSError());
     }
   } else {
     OSError os_error(-1, "Invalid argument", OSError::kUnknown);
     Dart_SetReturnValue(args, DartUtils::NewDartOSError(&os_error));
   }
 }
 
 
 void FUNCTION_NAME(File_Read)(Dart_NativeArguments args) {
-  File* file = GetFilePointer(Dart_GetNativeArgument(args, 0));
+  File* file = GetFile(args);
   ASSERT(file != NULL);
   Dart_Handle length_object = Dart_GetNativeArgument(args, 1);
   int64_t length = 0;
   if (DartUtils::GetInt64Value(length_object, &length)) {
     uint8_t* buffer = NULL;
     Dart_Handle external_array = IOBuffer::Allocate(length, &buffer);
     int64_t bytes_read = file->Read(reinterpret_cast<void*>(buffer), length);
     if (bytes_read < 0) {
       Dart_SetReturnValue(args, DartUtils::NewDartOSError());
     } else {
@@ skipping 20 matching lines @@
       }
     }
   } else {
     OSError os_error(-1, "Invalid argument", OSError::kUnknown);
     Dart_SetReturnValue(args, DartUtils::NewDartOSError(&os_error));
   }
 }
 
 
 void FUNCTION_NAME(File_ReadInto)(Dart_NativeArguments args) {
-  File* file = GetFilePointer(Dart_GetNativeArgument(args, 0));
+  File* file = GetFile(args);
   ASSERT(file != NULL);
   Dart_Handle buffer_obj = Dart_GetNativeArgument(args, 1);
   ASSERT(Dart_IsList(buffer_obj));
   // start and end arguments are checked in Dart code to be
   // integers and have the property that end <=
   // list.length. Therefore, it is safe to extract their value as
   // intptr_t.
   intptr_t start =
       DartUtils::GetIntptrValue(Dart_GetNativeArgument(args, 2));
   intptr_t end =
@@ skipping 14 matching lines @@
     } else {
       Dart_SetReturnValue(args, Dart_NewInteger(bytes_read));
     }
   } else {
     Dart_SetReturnValue(args, DartUtils::NewDartOSError());
   }
 }
 
 
 void FUNCTION_NAME(File_WriteFrom)(Dart_NativeArguments args) {
-  File* file = GetFilePointer(Dart_GetNativeArgument(args, 0));
+  File* file = GetFile(args);
   ASSERT(file != NULL);
 
   Dart_Handle buffer_obj = Dart_GetNativeArgument(args, 1);
 
   // Offset and length arguments are checked in Dart code to be
   // integers and have the property that (offset + length) <=
   // list.length. Therefore, it is safe to extract their value as
   // intptr_t.
   intptr_t start =
       DartUtils::GetIntptrValue(Dart_GetNativeArgument(args, 2));
@@ skipping 27 matching lines @@
 
   if (!success) {
     Dart_SetReturnValue(args, DartUtils::NewDartOSError());
   } else {
     Dart_SetReturnValue(args, Dart_Null());
   }
 }
 
 
 void FUNCTION_NAME(File_Position)(Dart_NativeArguments args) {
-  File* file = GetFilePointer(Dart_GetNativeArgument(args, 0));
+  File* file = GetFile(args);
   ASSERT(file != NULL);
   intptr_t return_value = file->Position();
   if (return_value >= 0) {
     Dart_SetReturnValue(args, Dart_NewInteger(return_value));
   } else {
     Dart_SetReturnValue(args, DartUtils::NewDartOSError());
   }
 }
 
 
 void FUNCTION_NAME(File_SetPosition)(Dart_NativeArguments args) {
-  File* file = GetFilePointer(Dart_GetNativeArgument(args, 0));
+  File* file = GetFile(args);
   ASSERT(file != NULL);
   int64_t position = 0;
   if (DartUtils::GetInt64Value(Dart_GetNativeArgument(args, 1), &position)) {
     if (file->SetPosition(position)) {
       Dart_SetReturnValue(args, Dart_True());
     } else {
       Dart_SetReturnValue(args, DartUtils::NewDartOSError());
     }
   } else {
     OSError os_error(-1, "Invalid argument", OSError::kUnknown);
     Dart_SetReturnValue(args, DartUtils::NewDartOSError(&os_error));
   }
 }
 
 
 void FUNCTION_NAME(File_Truncate)(Dart_NativeArguments args) {
-  File* file = GetFilePointer(Dart_GetNativeArgument(args, 0));
+  File* file = GetFile(args);
   ASSERT(file != NULL);
   int64_t length = 0;
   if (DartUtils::GetInt64Value(Dart_GetNativeArgument(args, 1), &length)) {
     if (file->Truncate(length)) {
       Dart_SetReturnValue(args, Dart_True());
     } else {
       Dart_SetReturnValue(args, DartUtils::NewDartOSError());
     }
   } else {
     OSError os_error(-1, "Invalid argument", OSError::kUnknown);
     Dart_SetReturnValue(args, DartUtils::NewDartOSError(&os_error));
   }
 }
 
 
 void FUNCTION_NAME(File_Length)(Dart_NativeArguments args) {
-  File* file = GetFilePointer(Dart_GetNativeArgument(args, 0));
+  File* file = GetFile(args);
   ASSERT(file != NULL);
   int64_t return_value = file->Length();
   if (return_value >= 0) {
     Dart_SetReturnValue(args, Dart_NewInteger(return_value));
   } else {
     Dart_SetReturnValue(args, DartUtils::NewDartOSError());
   }
 }
 
 
@@ skipping 15 matching lines @@
   int64_t return_value = File::LastModified(name);
   if (return_value >= 0) {
     Dart_SetReturnValue(args, Dart_NewInteger(return_value * kMSPerSecond));
   } else {
     Dart_SetReturnValue(args, DartUtils::NewDartOSError());
   }
 }
 
 
 void FUNCTION_NAME(File_Flush)(Dart_NativeArguments args) {
-  File* file = GetFilePointer(Dart_GetNativeArgument(args, 0));
+  File* file = GetFile(args);
   ASSERT(file != NULL);
   if (file->Flush()) {
     Dart_SetReturnValue(args, Dart_True());
   } else {
     Dart_SetReturnValue(args, DartUtils::NewDartOSError());
   }
 }
 
 
 void FUNCTION_NAME(File_Lock)(Dart_NativeArguments args) {
-  File* file = GetFilePointer(Dart_GetNativeArgument(args, 0));
+  File* file = GetFile(args);
   ASSERT(file != NULL);
   int64_t lock;
   int64_t start;
   int64_t end;
   if (DartUtils::GetInt64Value(Dart_GetNativeArgument(args, 1), &lock) &&
       DartUtils::GetInt64Value(Dart_GetNativeArgument(args, 2), &start) &&
       DartUtils::GetInt64Value(Dart_GetNativeArgument(args, 3), &end)) {
     if ((lock >= File::kLockMin) && (lock <= File::kLockMax) &&
         (start >= 0) && (end == -1 || end > start)) {
       if (file->Lock(static_cast<File::LockType>(lock), start, end)) {
@@ skipping 357 matching lines @@
     }
   }
   return CObject::IllegalArgumentError();
 }
 
 
 CObject* File::CloseRequest(const CObjectArray& request) {
   intptr_t return_value = -1;
   if ((request.Length() == 1) && request[0]->IsIntptr()) {
     File* file = CObjectToFilePointer(request[0]);
-    ASSERT(file != NULL);
-    delete file;
+    RefCntReleaseScope<File> rs(file);
     return_value = 0;
+    // We have retained a reference to the file here. Therefore the file's
+    // destructor can't be running. Since no further requests are dispatched by
+    // the Dart code after an async close call, this Close() can't be racing
+    // with any other call on the file. We don't do an extra Release(), and we
+    // don't delete the weak persistent handle. The file is closed here, but the
+    // memory will be cleaned up when the finalizer runs.
+    ASSERT(!file->IsClosed());
+    file->Close();
   }
   return new CObjectIntptr(CObject::NewIntptr(return_value));
 }
 
 
 CObject* File::PositionRequest(const CObjectArray& request) {
   if ((request.Length() == 1) && request[0]->IsIntptr()) {
     File* file = CObjectToFilePointer(request[0]);
-    ASSERT(file != NULL);
+    RefCntReleaseScope<File> rs(file);
     if (!file->IsClosed()) {
       intptr_t return_value = file->Position();
       if (return_value >= 0) {
         return new CObjectIntptr(CObject::NewIntptr(return_value));
       } else {
         return CObject::NewOSError();
       }
     } else {
       return CObject::FileClosedError();
     }
   }
   return CObject::IllegalArgumentError();
 }
 
 
 CObject* File::SetPositionRequest(const CObjectArray& request) {
   if ((request.Length() == 2) &&
       request[0]->IsIntptr() &&
       request[1]->IsInt32OrInt64()) {
     File* file = CObjectToFilePointer(request[0]);
-    ASSERT(file != NULL);
+    RefCntReleaseScope<File> rs(file);
     if (!file->IsClosed()) {
       int64_t position = CObjectInt32OrInt64ToInt64(request[1]);
       if (file->SetPosition(position)) {
         return CObject::True();
       } else {
         return CObject::NewOSError();
       }
     } else {
       return CObject::FileClosedError();
     }
   }
   return CObject::IllegalArgumentError();
 }
 
 
 CObject* File::TruncateRequest(const CObjectArray& request) {
   if ((request.Length() == 2) &&
       request[0]->IsIntptr() &&
       request[1]->IsInt32OrInt64()) {
     File* file = CObjectToFilePointer(request[0]);
-    ASSERT(file != NULL);
+    RefCntReleaseScope<File> rs(file);
     if (!file->IsClosed()) {
       int64_t length = CObjectInt32OrInt64ToInt64(request[1]);
       if (file->Truncate(length)) {
         return CObject::True();
       } else {
         return CObject::NewOSError();
       }
     } else {
       return CObject::FileClosedError();
     }
   }
   return CObject::IllegalArgumentError();
 }
 
 
 CObject* File::LengthRequest(const CObjectArray& request) {
   if ((request.Length() == 1) && request[0]->IsIntptr()) {
     File* file = CObjectToFilePointer(request[0]);
-    ASSERT(file != NULL);
+    RefCntReleaseScope<File> rs(file);
     if (!file->IsClosed()) {
       int64_t return_value = file->Length();
       if (return_value >= 0) {
         return new CObjectInt64(CObject::NewInt64(return_value));
       } else {
         return CObject::NewOSError();
       }
     } else {
       return CObject::FileClosedError();
     }
@@ skipping 26 matching lines @@
       return CObject::NewOSError();
     }
   }
   return CObject::IllegalArgumentError();
 }
 
 
 CObject* File::FlushRequest(const CObjectArray& request) {
   if ((request.Length() == 1) && request[0]->IsIntptr()) {
     File* file = CObjectToFilePointer(request[0]);
-    ASSERT(file != NULL);
+    RefCntReleaseScope<File> rs(file);
     if (!file->IsClosed()) {
       if (file->Flush()) {
         return CObject::True();
       } else {
         return CObject::NewOSError();
       }
     } else {
       return CObject::FileClosedError();
     }
   }
   return CObject::IllegalArgumentError();
 }
 
 
 CObject* File::ReadByteRequest(const CObjectArray& request) {
   if ((request.Length() == 1) && request[0]->IsIntptr()) {
     File* file = CObjectToFilePointer(request[0]);
-    ASSERT(file != NULL);
+    RefCntReleaseScope<File> rs(file);
     if (!file->IsClosed()) {
       uint8_t buffer;
       int64_t bytes_read = file->Read(reinterpret_cast<void*>(&buffer), 1);
       if (bytes_read > 0) {
         return new CObjectIntptr(CObject::NewIntptr(buffer));
       } else if (bytes_read == 0) {
         return new CObjectIntptr(CObject::NewIntptr(-1));
       } else {
         return CObject::NewOSError();
       }
     } else {
       return CObject::FileClosedError();
     }
   }
   return CObject::IllegalArgumentError();
 }
 
 
 CObject* File::WriteByteRequest(const CObjectArray& request) {
   if ((request.Length() == 2) &&
       request[0]->IsIntptr() &&
       request[1]->IsInt32OrInt64()) {
     File* file = CObjectToFilePointer(request[0]);
-    ASSERT(file != NULL);
+    RefCntReleaseScope<File> rs(file);
     if (!file->IsClosed()) {
       int64_t byte = CObjectInt32OrInt64ToInt64(request[1]);
       uint8_t buffer = static_cast<uint8_t>(byte & 0xff);
       bool success = file->WriteFully(reinterpret_cast<void*>(&buffer), 1);
       if (success) {
         return new CObjectInt64(CObject::NewInt64(1));
       } else {
         return CObject::NewOSError();
       }
     } else {
       return CObject::FileClosedError();
     }
   }
   return CObject::IllegalArgumentError();
 }
 
 
 CObject* File::ReadRequest(const CObjectArray& request) {
   if ((request.Length() == 2) &&
       request[0]->IsIntptr() &&
       request[1]->IsInt32OrInt64()) {
     File* file = CObjectToFilePointer(request[0]);
-    ASSERT(file != NULL);
+    RefCntReleaseScope<File> rs(file);
     if (!file->IsClosed()) {
       int64_t length = CObjectInt32OrInt64ToInt64(request[1]);
       Dart_CObject* io_buffer = CObject::NewIOBuffer(length);
       ASSERT(io_buffer != NULL);
       uint8_t* data = io_buffer->value.as_external_typed_data.data;
       int64_t bytes_read = file->Read(data, length);
       if (bytes_read >= 0) {
         CObjectExternalUint8Array* external_array =
             new CObjectExternalUint8Array(io_buffer);
         external_array->SetLength(bytes_read);
@@ skipping 11 matching lines @@
   }
   return CObject::IllegalArgumentError();
 }
 
 
 CObject* File::ReadIntoRequest(const CObjectArray& request) {
   if ((request.Length() == 2) &&
       request[0]->IsIntptr() &&
       request[1]->IsInt32OrInt64()) {
     File* file = CObjectToFilePointer(request[0]);
-    ASSERT(file != NULL);
+    RefCntReleaseScope<File> rs(file);
     if (!file->IsClosed()) {
       int64_t length = CObjectInt32OrInt64ToInt64(request[1]);
       Dart_CObject* io_buffer = CObject::NewIOBuffer(length);
       ASSERT(io_buffer != NULL);
       uint8_t* data = io_buffer->value.as_external_typed_data.data;
       int64_t bytes_read = file->Read(data, length);
       if (bytes_read >= 0) {
         CObjectExternalUint8Array* external_array =
             new CObjectExternalUint8Array(io_buffer);
         external_array->SetLength(bytes_read);
@@ skipping 39 matching lines @@
 }
 
 
 CObject* File::WriteFromRequest(const CObjectArray& request) {
   if ((request.Length() == 4) &&
       request[0]->IsIntptr() &&
       (request[1]->IsTypedData() || request[1]->IsArray()) &&
       request[2]->IsInt32OrInt64() &&
       request[3]->IsInt32OrInt64()) {
     File* file = CObjectToFilePointer(request[0]);
-    ASSERT(file != NULL);
+    RefCntReleaseScope<File> rs(file);
     if (!file->IsClosed()) {
       int64_t start = CObjectInt32OrInt64ToInt64(request[2]);
       int64_t end = CObjectInt32OrInt64ToInt64(request[3]);
       int64_t length = end - start;
       uint8_t* buffer_start;
       if (request[1]->IsTypedData()) {
         CObjectTypedData typed_data(request[1]);
         start = start * SizeInBytes(typed_data.Type());
         length = length * SizeInBytes(typed_data.Type());
         buffer_start = typed_data.Buffer() + start;
@@ skipping 142 matching lines @@
 }
 
 
 CObject* File::LockRequest(const CObjectArray& request) {
   if ((request.Length() == 4) &&
       request[0]->IsIntptr() &&
       request[1]->IsInt32OrInt64() &&
       request[2]->IsInt32OrInt64() &&
       request[3]->IsInt32OrInt64()) {
     File* file = CObjectToFilePointer(request[0]);
-    ASSERT(file != NULL);
+    RefCntReleaseScope<File> rs(file);
     if (!file->IsClosed()) {
       int64_t lock = CObjectInt32OrInt64ToInt64(request[1]);
       int64_t start = CObjectInt32OrInt64ToInt64(request[2]);
       int64_t end = CObjectInt32OrInt64ToInt64(request[3]);
       if (file->Lock(static_cast<File::LockType>(lock), start, end)) {
         return CObject::True();
       } else {
         return CObject::NewOSError();
       }
     } else {
       return CObject::FileClosedError();
     }
   }
   return CObject::IllegalArgumentError();
 }
 
 }  // namespace bin
 }  // namespace dart
 
 #endif  // !defined(DART_IO_DISABLED)