From 8e222b90b72772c69bd14dbd09be94ce067b1b1e Mon Sep 17 00:00:00 2001 From: Francois Rousseau Date: Wed, 13 Mar 2019 14:53:16 -0700 Subject: [PATCH] fix report size computation in prune condition today the attachments are not taken into account, but should on Linux and Fuchsia Bug: fuchsia:DX-1104 Tested:`fx run-test crashpad_test` for Fuchsia. Change-Id: I022331bdb09c637f40ff2ba2d711e301e211e86a Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/1518323 Reviewed-by: Scott Graham Commit-Queue: Francois Rousseau --- client/crash_report_database.cc | 3 +- client/crash_report_database.h | 5 ++ client/crash_report_database_generic.cc | 43 +++++++++++++-- client/crash_report_database_mac.mm | 8 +++ client/crash_report_database_test.cc | 68 +++++++++++++++++++++-- client/crash_report_database_win.cc | 14 ++++- client/prune_crash_reports.cc | 16 ++---- client/prune_crash_reports_test.cc | 71 +++++++++++-------------- 8 files changed, 166 insertions(+), 62 deletions(-) diff --git a/client/crash_report_database.cc b/client/crash_report_database.cc index d300a8f9..8fad1721 100644 --- a/client/crash_report_database.cc +++ b/client/crash_report_database.cc @@ -27,7 +27,8 @@ CrashReportDatabase::Report::Report() uploaded(false), last_upload_attempt_time(0), upload_attempts(0), - upload_explicitly_requested(false) {} + upload_explicitly_requested(false), + total_size(0u) {} CrashReportDatabase::NewReport::NewReport() : writer_(std::make_unique()), diff --git a/client/crash_report_database.h b/client/crash_report_database.h index 1d6a9ed0..f3cc31cc 100644 --- a/client/crash_report_database.h +++ b/client/crash_report_database.h @@ -15,6 +15,7 @@ #ifndef CRASHPAD_CLIENT_CRASH_REPORT_DATABASE_H_ #define CRASHPAD_CLIENT_CRASH_REPORT_DATABASE_H_ +#include #include #include @@ -98,6 +99,10 @@ class CrashReportDatabase { //! Whether this crash report was explicitly requested by user to be //! uploaded. This can be true only if report is in the 'pending' state. bool upload_explicitly_requested; + + //! The total size in bytes taken by the report, including any potential + //! attachments. + uint64_t total_size; }; //! \brief A crash report that is in the process of being written. diff --git a/client/crash_report_database_generic.cc b/client/crash_report_database_generic.cc index 8e932374..dab35a10 100644 --- a/client/crash_report_database_generic.cc +++ b/client/crash_report_database_generic.cc @@ -15,6 +15,7 @@ #include "client/crash_report_database.h" #include +#include #include #include @@ -160,6 +161,35 @@ class ScopedLockFile { DISALLOW_COPY_AND_ASSIGN(ScopedLockFile); }; +off_t GetFileSize(const base::FilePath& filepath) { + struct stat statbuf; + if (stat(filepath.value().c_str(), &statbuf) == 0) { + return statbuf.st_size; + } + PLOG(ERROR) << "stat " << filepath.value(); + return 0; +} + +void AddAttachmentSize(const base::FilePath& attachments_dir, uint64_t* size) { + // Early return if the attachment directory does not exist. + struct stat statbuf; + if (stat(attachments_dir.value().c_str(), &statbuf) != 0) { + return; + } + DirectoryReader reader; + if (!reader.Open(attachments_dir)) { + return; + } + base::FilePath attachment_filename; + DirectoryReader::Result result; + while ((result = reader.NextFile(&attachment_filename)) == + DirectoryReader::Result::kSuccess) { + const base::FilePath attachment_filepath( + attachments_dir.Append(attachment_filename)); + *size += GetFileSize(attachment_filepath); + } +} + } // namespace class CrashReportDatabaseGeneric : public CrashReportDatabase { @@ -253,7 +283,7 @@ class CrashReportDatabaseGeneric : public CrashReportDatabase { void RemoveAttachmentsByUUID(const UUID& uuid); // Reads the metadata for a report from path and returns it in report. - static bool ReadMetadata(const base::FilePath& path, Report* report); + bool ReadMetadata(const base::FilePath& path, Report* report); // Wraps ReadMetadata and removes the report from the database on failure. bool CleaningReadMetadata(const base::FilePath& path, Report* report); @@ -899,7 +929,6 @@ void CrashReportDatabaseGeneric::RemoveAttachmentsByUUID(const UUID& uuid) { LoggingRemoveDirectory(attachments_dir); } -// static bool CrashReportDatabaseGeneric::ReadMetadata(const base::FilePath& path, Report* report) { const base::FilePath metadata_path( @@ -910,7 +939,8 @@ bool CrashReportDatabaseGeneric::ReadMetadata(const base::FilePath& path, return false; } - if (!report->uuid.InitializeFromString( + UUID uuid; + if (!uuid.InitializeFromString( path.BaseName().RemoveFinalExtension().value())) { LOG(ERROR) << "Couldn't interpret report uuid"; return false; @@ -930,6 +960,12 @@ bool CrashReportDatabaseGeneric::ReadMetadata(const base::FilePath& path, return false; } + // Seed the total size with the main report size and then add the sizes of any + // potential attachments. + uint64_t total_size = GetFileSize(path); + AddAttachmentSize(AttachmentsPath(uuid), &total_size); + + report->uuid = uuid; report->upload_attempts = metadata.upload_attempts; report->last_upload_attempt_time = metadata.last_upload_attempt_time; report->creation_time = metadata.creation_time; @@ -937,6 +973,7 @@ bool CrashReportDatabaseGeneric::ReadMetadata(const base::FilePath& path, report->upload_explicitly_requested = (metadata.attributes & kAttributeUploadExplicitlyRequested) != 0; report->file_path = path; + report->total_size = total_size; return true; } diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm index 414dd5a4..1f8fec9b 100644 --- a/client/crash_report_database_mac.mm +++ b/client/crash_report_database_mac.mm @@ -682,6 +682,14 @@ bool CrashReportDatabaseMac::ReadReportMetadataLocked( return false; } + // There are no attachments on Mac so the total size is the main report size. + struct stat statbuf; + if (stat(path.value().c_str(), &statbuf) != 0) { + PLOG(ERROR) << "stat " << path.value(); + return false; + } + report->total_size = statbuf.st_size; + return true; } diff --git a/client/crash_report_database_test.cc b/client/crash_report_database_test.cc index 2dbb4fc1..ca66bce6 100644 --- a/client/crash_report_database_test.cc +++ b/client/crash_report_database_test.cc @@ -31,8 +31,7 @@ namespace { class CrashReportDatabaseTest : public testing::Test { public: - CrashReportDatabaseTest() { - } + CrashReportDatabaseTest() {} protected: // testing::Test: @@ -41,9 +40,7 @@ class CrashReportDatabaseTest : public testing::Test { ASSERT_TRUE(db_); } - void ResetDatabase() { - db_.reset(); - } + void ResetDatabase() { db_.reset(); } CrashReportDatabase* db() { return db_.get(); } base::FilePath path() const { @@ -101,6 +98,7 @@ class CrashReportDatabaseTest : public testing::Test { EXPECT_EQ(report.last_upload_attempt_time, 0); EXPECT_EQ(report.upload_attempts, 0); EXPECT_FALSE(report.upload_explicitly_requested); + EXPECT_GE(report.total_size, 0u); } void RelocateDatabase() { @@ -835,6 +833,66 @@ TEST_F(CrashReportDatabaseTest, CleanBrokenDatabase) { } #endif // !OS_MACOSX && !OS_WIN +TEST_F(CrashReportDatabaseTest, TotalSize_MainReportOnly) { + std::unique_ptr new_report; + ASSERT_EQ(db()->PrepareNewCrashReport(&new_report), + CrashReportDatabase::kNoError); + + // Main report. + static constexpr char main_report_data[] = "dlbvandslhb"; + ASSERT_TRUE( + new_report->Writer()->Write(main_report_data, sizeof(main_report_data))); + + UUID uuid; + ASSERT_EQ(db()->FinishedWritingCrashReport(std::move(new_report), &uuid), + CrashReportDatabase::kNoError); + + CrashReportDatabase::Report report; + ASSERT_EQ(db()->LookUpCrashReport(uuid, &report), + CrashReportDatabase::kNoError); + + EXPECT_EQ(report.total_size, sizeof(main_report_data)); +} + +TEST_F(CrashReportDatabaseTest, GetReportSize_RightSizeWithAttachments) { +#if defined(OS_MACOSX) || defined(OS_WIN) + // Attachments aren't supported on Mac and Windows yet. + return; +#else + std::unique_ptr new_report; + ASSERT_EQ(db()->PrepareNewCrashReport(&new_report), + CrashReportDatabase::kNoError); + + // Main report. + static constexpr char main_report_data[] = "dlbvandslhb"; + ASSERT_TRUE( + new_report->Writer()->Write(main_report_data, sizeof(main_report_data))); + + // First attachment. + FileWriter* attachment_1 = new_report->AddAttachment("my_attachment_1"); + ASSERT_NE(attachment_1, nullptr); + static constexpr char attachment_1_data[] = "vKDnidhvbiudshoihbvdsoiuh nhh"; + attachment_1->Write(attachment_1_data, sizeof(attachment_1_data)); + + // Second attachment. + FileWriter* attachment_2 = new_report->AddAttachment("my_attachment_2"); + ASSERT_NE(attachment_2, nullptr); + static constexpr char attachment_2_data[] = "sgvsvgusiyguysigfkhpmo-["; + attachment_2->Write(attachment_2_data, sizeof(attachment_2_data)); + + UUID uuid; + ASSERT_EQ(db()->FinishedWritingCrashReport(std::move(new_report), &uuid), + CrashReportDatabase::kNoError); + + CrashReportDatabase::Report report; + ASSERT_EQ(db()->LookUpCrashReport(uuid, &report), + CrashReportDatabase::kNoError); + EXPECT_EQ(report.total_size, + sizeof(main_report_data) + sizeof(attachment_1_data) + + sizeof(attachment_2_data)); +#endif +} + } // namespace } // namespace test } // namespace crashpad diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc index 89677706..91179461 100644 --- a/client/crash_report_database_win.cc +++ b/client/crash_report_database_win.cc @@ -458,7 +458,19 @@ void Metadata::Read() { LOG(ERROR) << "invalid string table index"; return; } - reports.push_back(ReportDisk(record, report_dir_, string_table)); + ReportDisk report_disk(record, report_dir_, string_table); + + // There are no attachments on Windows so the total size is the main + // report size. + struct _stati64 statbuf; + if (_wstat64(report_disk.file_path.value().c_str(), &statbuf) == 0) { + report_disk.total_size = statbuf.st_size; + } else { + PLOG(ERROR) << "stat " + << base::UTF16ToUTF8(report_disk.file_path.value()); + } + + reports.push_back(report_disk); } } reports_.swap(reports); diff --git a/client/prune_crash_reports.cc b/client/prune_crash_reports.cc index d045eb6a..c6fa389b 100644 --- a/client/prune_crash_reports.cc +++ b/client/prune_crash_reports.cc @@ -96,19 +96,9 @@ DatabaseSizePruneCondition::~DatabaseSizePruneCondition() {} bool DatabaseSizePruneCondition::ShouldPruneReport( const CrashReportDatabase::Report& report) { -#if defined(OS_POSIX) - struct stat statbuf; - if (stat(report.file_path.value().c_str(), &statbuf) == 0) { -#elif defined(OS_WIN) - struct _stati64 statbuf; - if (_wstat64(report.file_path.value().c_str(), &statbuf) == 0) { -#else -#error "Not implemented" -#endif - // Round up fractional KB to the next 1-KB boundary. - measured_size_in_kb_ += - static_cast((statbuf.st_size + 1023) / 1024); - } + // Round up fractional KB to the next 1-KB boundary. + measured_size_in_kb_ += + static_cast((report.total_size + 1023) / 1024); return measured_size_in_kb_ > max_size_in_kb_; } diff --git a/client/prune_crash_reports_test.cc b/client/prune_crash_reports_test.cc index 9f5852e6..c8c43444 100644 --- a/client/prune_crash_reports_test.cc +++ b/client/prune_crash_reports_test.cc @@ -81,56 +81,49 @@ TEST(PruneCrashReports, AgeCondition) { } TEST(PruneCrashReports, SizeCondition) { - ScopedTempDir temp_dir; - CrashReportDatabase::Report report_1k; - report_1k.file_path = temp_dir.path().Append(FILE_PATH_LITERAL("file1024")); + report_1k.total_size = 1024u; CrashReportDatabase::Report report_3k; - report_3k.file_path = temp_dir.path().Append(FILE_PATH_LITERAL("file3072")); + report_3k.total_size = 1024u * 3u; + CrashReportDatabase::Report report_unset_size; { - ScopedFileHandle scoped_file_1k( - LoggingOpenFileForWrite(report_1k.file_path, - FileWriteMode::kCreateOrFail, - FilePermissions::kOwnerOnly)); - ASSERT_TRUE(scoped_file_1k.is_valid()); - - std::string string; - for (int i = 0; i < 128; ++i) - string.push_back(static_cast(i)); - - for (size_t i = 0; i < 1024; i += string.size()) { - ASSERT_TRUE(LoggingWriteFile(scoped_file_1k.get(), - string.c_str(), string.length())); - } - - ScopedFileHandle scoped_file_3k( - LoggingOpenFileForWrite(report_3k.file_path, - FileWriteMode::kCreateOrFail, - FilePermissions::kOwnerOnly)); - ASSERT_TRUE(scoped_file_3k.is_valid()); - - for (size_t i = 0; i < 3072; i += string.size()) { - ASSERT_TRUE(LoggingWriteFile(scoped_file_3k.get(), - string.c_str(), string.length())); - } - } - - { - DatabaseSizePruneCondition condition(1); + DatabaseSizePruneCondition condition(/*max_size_in_kb=*/1); + // |report_1k| should not be pruned as the cumulated size is not past 1kB + // yet. EXPECT_FALSE(condition.ShouldPruneReport(report_1k)); - EXPECT_TRUE(condition.ShouldPruneReport(report_1k)); - } - - { - DatabaseSizePruneCondition condition(1); + // |report_3k| should be pruned as the cumulated size is now past 1kB. EXPECT_TRUE(condition.ShouldPruneReport(report_3k)); } { - DatabaseSizePruneCondition condition(6); + DatabaseSizePruneCondition condition(/*max_size_in_kb=*/1); + // |report_3k| should be pruned as the cumulated size is already past 1kB. + EXPECT_TRUE(condition.ShouldPruneReport(report_3k)); + } + + { + DatabaseSizePruneCondition condition(/*max_size_in_kb=*/6); + // |report_3k| should not be pruned as the cumulated size is not past 6kB + // yet. EXPECT_FALSE(condition.ShouldPruneReport(report_3k)); + // |report_3k| should not be pruned as the cumulated size is not past 6kB + // yet. EXPECT_FALSE(condition.ShouldPruneReport(report_3k)); + // |report_1k| should be pruned as the cumulated size is now past 6kB. + EXPECT_TRUE(condition.ShouldPruneReport(report_1k)); + } + + { + DatabaseSizePruneCondition condition(/*max_size_in_kb=*/0); + // |report_unset_size| should not be pruned as its size is 0, regardless of + // how many times we try to prune it. + EXPECT_FALSE(condition.ShouldPruneReport(report_unset_size)); + EXPECT_FALSE(condition.ShouldPruneReport(report_unset_size)); + EXPECT_FALSE(condition.ShouldPruneReport(report_unset_size)); + EXPECT_FALSE(condition.ShouldPruneReport(report_unset_size)); + EXPECT_FALSE(condition.ShouldPruneReport(report_unset_size)); + // |report_1k| should be pruned as the cumulated size is now past 0kB. EXPECT_TRUE(condition.ShouldPruneReport(report_1k)); } }