From 8b6e39696f434e63e8684d9a920062d62cc8054f Mon Sep 17 00:00:00 2001 From: Slava Egorov Date: Wed, 28 Jun 2023 13:06:51 +0000 Subject: [PATCH] [vm/io] Avoid leaking Handle::data_ready_ on Windows. Handle::data_ready_ contains bytes which are ready to be sent to Dart side. Not all code paths fully drain this buffer and delete it before destroying the handle, for example directory watch implementation was prone to leaking data_ready_ when subscription was cancelled. This CL switches the code to use unique_ptr to hold on to data_ready_ which makes sure that it is deleted when Handle is destroyed. The code would benefit from holding all OverlappedBuffer instances through unique_ptr but that is a much larger refactoring which we leave for a later date. Fixes https://github.com/dart-lang/sdk/issues/52715 TEST=standalone{,_2}/regress_52715_test Bug: 52715 Cq-Include-Trybots: luci.dart.try:vm-win-release-x64-try,vm-win-debug-x64-try,vm-aot-win-release-x64-try,analyzer-win-release-try Change-Id: Ie8d728b823de7e8f9de1489898e270580c2af269 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/311841 Commit-Queue: Slava Egorov Commit-Queue: Martin Kustermann Reviewed-by: Martin Kustermann Auto-Submit: Slava Egorov --- runtime/bin/eventhandler_win.cc | 10 +++------ runtime/bin/eventhandler_win.h | 7 +++--- tests/standalone/regress_52715_test.dart | 24 ++++++++++++++++++++ tests/standalone_2/regress_52715_test.dart | 26 ++++++++++++++++++++++ 4 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 tests/standalone/regress_52715_test.dart create mode 100644 tests/standalone_2/regress_52715_test.dart diff --git a/runtime/bin/eventhandler_win.cc b/runtime/bin/eventhandler_win.cc index 07314cd9ce56..df164ea89a04 100644 --- a/runtime/bin/eventhandler_win.cc +++ b/runtime/bin/eventhandler_win.cc @@ -119,7 +119,7 @@ Handle::Handle(intptr_t handle) handle_(reinterpret_cast(handle)), completion_port_(INVALID_HANDLE_VALUE), event_handler_(nullptr), - data_ready_(nullptr), + data_ready_(), pending_read_(nullptr), pending_write_(nullptr), last_error_(NOERROR), @@ -221,7 +221,7 @@ void Handle::ReadComplete(OverlappedBuffer* buffer) { ASSERT(pending_read_ == buffer); ASSERT(data_ready_ == nullptr); if (!IsClosing()) { - data_ready_ = pending_read_; + data_ready_.reset(pending_read_); } else { OverlappedBuffer::DisposeBuffer(buffer); } @@ -665,7 +665,6 @@ intptr_t Handle::Read(void* buffer, intptr_t num_bytes) { num_bytes = data_ready_->Read(buffer, Utils::Minimum(num_bytes, INT_MAX)); if (data_ready_->IsEmpty()) { - OverlappedBuffer::DisposeBuffer(data_ready_); data_ready_ = nullptr; if (!IsClosing() && !IsClosedRead()) { IssueRead(); @@ -694,7 +693,6 @@ intptr_t Handle::RecvFrom(void* buffer, } // Always dispose of the buffer, as UDP messages must be read in their // entirety to match how recvfrom works in a socket. - OverlappedBuffer::DisposeBuffer(data_ready_); data_ready_ = nullptr; if (!IsClosing() && !IsClosedRead()) { IssueRecvFrom(); @@ -975,9 +973,7 @@ void ClientSocket::IssueDisconnect() { void ClientSocket::DisconnectComplete(OverlappedBuffer* buffer) { OverlappedBuffer::DisposeBuffer(buffer); closesocket(socket()); - if (data_ready_ != nullptr) { - OverlappedBuffer::DisposeBuffer(data_ready_); - } + data_ready_ = nullptr; mark_closed(); #if defined(DEBUG) disconnecting_--; diff --git a/runtime/bin/eventhandler_win.h b/runtime/bin/eventhandler_win.h index f7242f944f18..11210a80749b 100644 --- a/runtime/bin/eventhandler_win.h +++ b/runtime/bin/eventhandler_win.h @@ -100,6 +100,8 @@ class OverlappedBuffer { void set_data_length(int data_length) { data_length_ = data_length; } + void operator delete(void* buffer) { free(buffer); } + private: OverlappedBuffer(int buffer_size, Operation operation) : operation_(operation), buflen_(buffer_size) { @@ -130,8 +132,6 @@ class OverlappedBuffer { return malloc(size + buffer_size); } - void operator delete(void* buffer) { free(buffer); } - // Allocate an overlapped buffer for thse specified amount of data and // operation. Some operations need additional buffer space, which is // handled by this method. @@ -272,7 +272,8 @@ class Handle : public ReferenceCounted, public DescriptorInfoBase { HANDLE completion_port_; EventHandlerImplementation* event_handler_; - OverlappedBuffer* data_ready_; // Buffer for data ready to be read. + std::unique_ptr + data_ready_; // Buffer for data ready to be read. OverlappedBuffer* pending_read_; // Buffer for pending read. OverlappedBuffer* pending_write_; // Buffer for pending write diff --git a/tests/standalone/regress_52715_test.dart b/tests/standalone/regress_52715_test.dart new file mode 100644 index 000000000000..fb6341030a0f --- /dev/null +++ b/tests/standalone/regress_52715_test.dart @@ -0,0 +1,24 @@ +// Copyright (c) 2023, 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. + +// Verify that cancelled [Directory.watch] subscriptions do not waste memory. + +import 'dart:io'; + +import 'package:expect/expect.dart'; + +void main() async { + final startRss = ProcessInfo.currentRss; + + for (var i = 0; i < 1024; i++) { + final subscription = Directory.systemTemp.watch().listen((event) {}); + await subscription.cancel(); + } + + final endRss = ProcessInfo.currentRss; + final allocatedBytes = (endRss - startRss); + final limit = 10 * 1024 * 1024; + Expect.isTrue(allocatedBytes < limit, + 'expected VM RSS growth to be below ${limit} but got ${allocatedBytes}'); +} diff --git a/tests/standalone_2/regress_52715_test.dart b/tests/standalone_2/regress_52715_test.dart new file mode 100644 index 000000000000..6bf7043743c8 --- /dev/null +++ b/tests/standalone_2/regress_52715_test.dart @@ -0,0 +1,26 @@ +// Copyright (c) 2023, 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. + +// Verify that cancelled [Directory.watch] subscriptions do not waste memory. + +// @dart=2.9 + +import 'dart:io'; + +import 'package:expect/expect.dart'; + +void main() async { + final startRss = ProcessInfo.currentRss; + + for (var i = 0; i < 1024; i++) { + final subscription = Directory.systemTemp.watch().listen((event) {}); + await subscription.cancel(); + } + + final endRss = ProcessInfo.currentRss; + final allocatedBytes = (endRss - startRss); + final limit = 10 * 1024 * 1024; + Expect.isTrue(allocatedBytes < limit, + 'expected VM RSS growth to be below ${limit} but got ${allocatedBytes}'); +}