From afeb19f1d24f292c8a26e14f41ec240cf8b8821b Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Thu, 11 Nov 2021 06:32:46 -0500 Subject: [PATCH] ios: Limit intermediate dump processing to 20 files. Because the intermediate dump directory is expected to be shared, mitigate any spamming by limiting this to 20. Prioritize our bundle id intermediate dumps first. Bug: crashpad: 31 Change-Id: I2888431b8bd2d94f481d2f4ec6e032882dad9698 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3261747 Commit-Queue: Justin Cohen Reviewed-by: Mark Mentovai --- client/BUILD.gn | 1 + client/ios_handler/in_process_handler.cc | 34 +++- client/ios_handler/in_process_handler.h | 4 + client/ios_handler/in_process_handler_test.cc | 150 ++++++++++++++++++ 4 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 client/ios_handler/in_process_handler_test.cc diff --git a/client/BUILD.gn b/client/BUILD.gn index b60e9044..1a83efa2 100644 --- a/client/BUILD.gn +++ b/client/BUILD.gn @@ -170,6 +170,7 @@ source_set("client_test") { sources += [ "crashpad_client_ios_test.mm", "ios_handler/exception_processor_test.mm", + "ios_handler/in_process_handler_test.cc", "ios_handler/in_process_intermediate_dump_handler_test.cc", ] } diff --git a/client/ios_handler/in_process_handler.cc b/client/ios_handler/in_process_handler.cc index 6e440bef..d57b5f29 100644 --- a/client/ios_handler/in_process_handler.cc +++ b/client/ios_handler/in_process_handler.cc @@ -54,7 +54,10 @@ namespace internal { InProcessHandler::InProcessHandler() = default; InProcessHandler::~InProcessHandler() { - upload_thread_->Stop(); + if (upload_thread_started_ && upload_thread_) { + upload_thread_->Stop(); + } + prune_thread_->Stop(); } bool InProcessHandler::Initialize( @@ -225,13 +228,23 @@ std::vector InProcessHandler::PendingFiles() { } base::FilePath file; DirectoryReader::Result result; + + // Because the intermediate dump directory is expected to be shared, + // mitigate any spamming by limiting this to |kMaxPendingFiles|. + constexpr size_t kMaxPendingFiles = 20; + + // Track other application bundles separately, so they don't spam our + // intermediate dumps into never getting processed. + std::vector other_files; + while ((result = reader.NextFile(&file)) == DirectoryReader::Result::kSuccess) { // Don't try to process files marked as 'locked' from a different bundle id. - if (file.value().compare(0, + bool bundle_match = + file.value().compare(0, bundle_identifier_and_seperator_.size(), - bundle_identifier_and_seperator_) != 0 && - file.FinalExtension() == kLockedExtension) { + bundle_identifier_and_seperator_) == 0; + if (!bundle_match && file.FinalExtension() == kLockedExtension) { continue; } @@ -242,8 +255,19 @@ std::vector InProcessHandler::PendingFiles() { // Otherwise, include any other unlocked, or locked files matching // |bundle_identifier_and_seperator_|. - files.push_back(file); + if (bundle_match) { + files.push_back(file); + if (files.size() >= kMaxPendingFiles) + return files; + } else { + other_files.push_back(file); + } } + + auto end_iterator = + other_files.begin() + + std::min(kMaxPendingFiles - files.size(), other_files.size()); + files.insert(files.end(), other_files.begin(), end_iterator); return files; } diff --git a/client/ios_handler/in_process_handler.h b/client/ios_handler/in_process_handler.h index 888337b9..e586219a 100644 --- a/client/ios_handler/in_process_handler.h +++ b/client/ios_handler/in_process_handler.h @@ -180,7 +180,11 @@ class InProcessHandler { } void SaveSnapshot(ProcessSnapshotIOSIntermediateDump& process_snapshot); + + // Process a maximum of 20 pending intermediate dumps. Dumps named with our + // bundle id get first priority to prevent spamming. std::vector PendingFiles(); + bool OpenNewFile(); void PostReportCleanup(); diff --git a/client/ios_handler/in_process_handler_test.cc b/client/ios_handler/in_process_handler_test.cc new file mode 100644 index 00000000..13dd366d --- /dev/null +++ b/client/ios_handler/in_process_handler_test.cc @@ -0,0 +1,150 @@ +// Copyright 2021 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "client/ios_handler/in_process_handler.h" + +#include "gtest/gtest.h" +#include "test/scoped_temp_dir.h" +#include "test/test_paths.h" +#include "util/file/directory_reader.h" +#include "util/file/filesystem.h" + +namespace crashpad { +namespace test { +namespace { + +bool CreateFile(const base::FilePath& file) { + ScopedFileHandle fd(LoggingOpenFileForWrite( + file, FileWriteMode::kCreateOrFail, FilePermissions::kOwnerOnly)); + EXPECT_TRUE(fd.is_valid()); + return fd.is_valid(); +} + +class InProcessHandlerTest : public testing::Test { + protected: + // testing::Test: + + void SetUp() override { + ASSERT_TRUE( + in_process_handler_.Initialize(temp_dir_.path(), "", {}, system_data_)); + pending_dir_ = temp_dir_.path().Append("pending-serialized-ios-dump"); + bundle_identifier_and_seperator_ = system_data_.BundleIdentifier() + "@"; + } + + const auto& path() const { return pending_dir_; } + auto& handler() { return in_process_handler_; } + + void CreateFiles(int files, int other_files) { + base::FilePath::StringType file_prepend = + FILE_PATH_LITERAL(bundle_identifier_and_seperator_); + base::FilePath::StringType file_name = FILE_PATH_LITERAL("file"); + for (int i = 0; i < files; i++) { + std::string i_str = std::to_string(i); + base::FilePath file(file_prepend + file_name + i_str); + CreateFile(path().Append(file)); + } + + for (int i = 0; i < other_files; i++) { + std::string i_str = std::to_string(i); + base::FilePath file(file_name + i_str); + CreateFile(path().Append(file)); + } + } + + void VerifyRemainingFileCount(int expected_files_count, + int expected_other_files_count) { + DirectoryReader reader; + ASSERT_TRUE(reader.Open(path())); + DirectoryReader::Result result; + base::FilePath filename; + int files_count = 0; + int other_files_count = 0; + while ((result = reader.NextFile(&filename)) == + DirectoryReader::Result::kSuccess) { + bool bundle_match = + filename.value().compare(0, + bundle_identifier_and_seperator_.size(), + bundle_identifier_and_seperator_) == 0; + if (bundle_match) { + files_count++; + } else { + other_files_count++; + } + } + EXPECT_EQ(expected_files_count, files_count); + EXPECT_EQ(expected_other_files_count, other_files_count); + } + + void ClearFiles() { + DirectoryReader reader; + ASSERT_TRUE(reader.Open(path())); + DirectoryReader::Result result; + base::FilePath filename; + while ((result = reader.NextFile(&filename)) == + DirectoryReader::Result::kSuccess) { + LoggingRemoveFile(path().Append(filename)); + } + } + + private: + ScopedTempDir temp_dir_; + base::FilePath pending_dir_; + std::string bundle_identifier_and_seperator_; + internal::IOSSystemDataCollector system_data_; + internal::InProcessHandler in_process_handler_; +}; + +TEST_F(InProcessHandlerTest, TestPendingFileLimit) { + // Clear this first to blow away the pending file held by InProcessHandler. + ClearFiles(); + + // Only process other app files. + CreateFiles(0, 20); + handler().ProcessIntermediateDumps({}); + VerifyRemainingFileCount(0, 0); + ClearFiles(); + + // Only process our app files. + CreateFiles(20, 20); + handler().ProcessIntermediateDumps({}); + VerifyRemainingFileCount(0, 20); + ClearFiles(); + + // Process all of our files and 10 remaining. + CreateFiles(10, 30); + handler().ProcessIntermediateDumps({}); + VerifyRemainingFileCount(0, 20); + ClearFiles(); + + // Process 20 our files, leaving 10 remaining, and all other files remaining. + CreateFiles(30, 10); + handler().ProcessIntermediateDumps({}); + VerifyRemainingFileCount(10, 10); + ClearFiles(); + + CreateFiles(0, 0); + handler().ProcessIntermediateDumps({}); + VerifyRemainingFileCount(0, 0); + ClearFiles(); + + CreateFiles(10, 0); + handler().ProcessIntermediateDumps({}); + VerifyRemainingFileCount(0, 0); + ClearFiles(); + +} + +} // namespace +} // namespace test +} // namespace crashpad