From 2071abaeb4deac03c5032581f758c0f20862adf2 Mon Sep 17 00:00:00 2001 From: Ben Hamilton Date: Tue, 17 May 2022 15:18:21 -0600 Subject: [PATCH] [ios] Fix FD leak in IOSIntermediateDumpWriter IOSIntermediateDumpWriter::Close() is intended to close the FD opened by the in-process handler. Currently, InProcessHandler::ScopedLockedWriter::~ScopedLockedWriter() does invoke IOSIntermediateDumpWriter::Close(). However, InProcessHandler::Initialize() invokes the utility CreateWriterWithPath() which directly creates an IOSIntermediateDumpWriter. It neither uses ScopedLockedWriter nor invokes Close(). This fixes the issue by: 1) Making IOSIntermediateDumpWriter::~IOSIntermediateDumpWriter() DCHECK() that it's closed 2) Calling IOSIntermediateDumpWriter::Close() from InProcessHandler::~InProcessHandler() and from test files Change-Id: Ibfede0a3d2aeac948c7ff3d56445e13d1a4028b5 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3648710 Commit-Queue: Justin Cohen Reviewed-by: Justin Cohen --- client/ios_handler/in_process_handler.cc | 3 +++ .../in_process_intermediate_dump_handler_test.cc | 1 + .../process_snapshot_ios_intermediate_dump_test.cc | 1 + util/ios/ios_intermediate_dump_reader_test.cc | 1 + util/ios/ios_intermediate_dump_writer.cc | 12 +++++++++++- util/ios/ios_intermediate_dump_writer.h | 6 +++++- util/ios/ios_intermediate_dump_writer_test.cc | 1 + 7 files changed, 23 insertions(+), 2 deletions(-) diff --git a/client/ios_handler/in_process_handler.cc b/client/ios_handler/in_process_handler.cc index f13b140d..92bb72e1 100644 --- a/client/ios_handler/in_process_handler.cc +++ b/client/ios_handler/in_process_handler.cc @@ -62,6 +62,9 @@ namespace internal { InProcessHandler::InProcessHandler() = default; InProcessHandler::~InProcessHandler() { + if (cached_writer_) { + cached_writer_->Close(); + } UpdatePruneAndUploadThreads(false); } diff --git a/client/ios_handler/in_process_intermediate_dump_handler_test.cc b/client/ios_handler/in_process_intermediate_dump_handler_test.cc index e61a604d..bccc930d 100644 --- a/client/ios_handler/in_process_intermediate_dump_handler_test.cc +++ b/client/ios_handler/in_process_intermediate_dump_handler_test.cc @@ -49,6 +49,7 @@ class InProcessIntermediateDumpHandlerTest : public testing::Test { } void TearDown() override { + EXPECT_TRUE(writer_->Close()); writer_.reset(); EXPECT_FALSE(IsRegularFile(path_)); } diff --git a/snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc b/snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc index 1a994bdb..44473ccb 100644 --- a/snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc +++ b/snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc @@ -61,6 +61,7 @@ class ProcessSnapshotIOSIntermediateDumpTest : public testing::Test { } void TearDown() override { + EXPECT_TRUE(writer_->Close()); writer_.reset(); EXPECT_FALSE(IsRegularFile(path_)); } diff --git a/util/ios/ios_intermediate_dump_reader_test.cc b/util/ios/ios_intermediate_dump_reader_test.cc index a66d6c3d..7dfecdf9 100644 --- a/util/ios/ios_intermediate_dump_reader_test.cc +++ b/util/ios/ios_intermediate_dump_reader_test.cc @@ -53,6 +53,7 @@ class IOSIntermediateDumpReaderTest : public testing::Test { } void TearDown() override { + ASSERT_TRUE(writer_->Close()); fd_.reset(); writer_.reset(); EXPECT_FALSE(IsRegularFile(path_)); diff --git a/util/ios/ios_intermediate_dump_writer.cc b/util/ios/ios_intermediate_dump_writer.cc index cd4d99d2..cd236dfa 100644 --- a/util/ios/ios_intermediate_dump_writer.cc +++ b/util/ios/ios_intermediate_dump_writer.cc @@ -17,6 +17,7 @@ #include #include +#include "base/check.h" #include "base/posix/eintr_wrapper.h" #include "build/build_config.h" #include "util/ios/raw_logging.h" @@ -50,6 +51,10 @@ bool RawLoggingCloseFile(int fd) { return rv == 0; } +IOSIntermediateDumpWriter::~IOSIntermediateDumpWriter() { + DCHECK_EQ(fd_, -1) << "Call Close() before this object is destroyed."; +} + bool IOSIntermediateDumpWriter::Open(const base::FilePath& path) { // Set data protection class D (No protection). A file with this type of // protection can be read from or written to at any time. @@ -71,7 +76,12 @@ bool IOSIntermediateDumpWriter::Open(const base::FilePath& path) { } bool IOSIntermediateDumpWriter::Close() { - return RawLoggingCloseFile(fd_); + if (fd_ < 0) { + return true; + } + int fd = fd_; + fd_ = -1; + return RawLoggingCloseFile(fd); } bool IOSIntermediateDumpWriter::ArrayMapStart() { diff --git a/util/ios/ios_intermediate_dump_writer.h b/util/ios/ios_intermediate_dump_writer.h index 836fbe7a..d4f7a7b7 100644 --- a/util/ios/ios_intermediate_dump_writer.h +++ b/util/ios/ios_intermediate_dump_writer.h @@ -39,12 +39,14 @@ namespace internal { //! Note: All methods are `RUNS-DURING-CRASH`. class IOSIntermediateDumpWriter final { public: - IOSIntermediateDumpWriter() = default; + IOSIntermediateDumpWriter() : fd_(-1) { } IOSIntermediateDumpWriter(const IOSIntermediateDumpWriter&) = delete; IOSIntermediateDumpWriter& operator=(const IOSIntermediateDumpWriter&) = delete; + ~IOSIntermediateDumpWriter(); + //! \brief Command instructions for the intermediate dump reader. enum class CommandType : uint8_t { //! \brief Indicates a new map, followed by associated key. @@ -73,6 +75,8 @@ class IOSIntermediateDumpWriter final { //! \brief Open and lock an intermediate dump file. This is the only method //! in the writer class that is generally run outside of a crash. //! + //! The client must invoke `Close()` before this object is destroyed. + //! //! \param[in] path The path to the intermediate dump. //! //! \return On success, returns `true`, otherwise returns `false`. diff --git a/util/ios/ios_intermediate_dump_writer_test.cc b/util/ios/ios_intermediate_dump_writer_test.cc index 24f56efd..055ae70c 100644 --- a/util/ios/ios_intermediate_dump_writer_test.cc +++ b/util/ios/ios_intermediate_dump_writer_test.cc @@ -41,6 +41,7 @@ class IOSIntermediateDumpWriterTest : public testing::Test { } void TearDown() override { + EXPECT_TRUE(writer_->Close()); writer_.reset(); EXPECT_EQ(unlink(path_.value().c_str()), 0) << ErrnoMessage("unlink"); }