mac: Prevent the same report from being uploaded multiple times

With multiple crashpad_handlers running out of the same database, it was possible for more than one to attempt to upload the same report. Nothing ensured that the reports remained pending between the calls to CrashReportDatabaseMac::GetPendingReports() and CrashReportDatabaseMac::GetReportForUploading(). The Windows equivalent did not share this bug, but it would return kBusyError. kReportNotFound is a better code. Test: crashpad_client_test CrashReportDatabaseTest.* Change-Id: Ieaee7f94ca8e6f2606d000bd2ba508d3cfa2fe07 Reviewed-on: https://chromium-review.googlesource.com/473928 Reviewed-by: Robert Sesek <[email protected]>
sbahra · Apr 13, 2017 · bc7c6e2 · bc7c6e2
1 parent 1f28a12
commit bc7c6e2
Show file tree

Hide file tree

Showing 5 changed files with 76 additions and 17 deletions.
diff --git a/client/crash_report_database.h b/client/crash_report_database.h
@@ -144,6 +144,11 @@ class CrashReportDatabase {
     kNoError = 0,
 
     //! \brief The report that was requested could not be located.
+    //!
+    //! This may occur when the report is present in the database but not in a
+    //! state appropriate for the requested operation, for example, if
+    //! GetReportForUploading() is called to obtain report that’s already in the
+    //! completed state.
     kReportNotFound,
 
     //! \brief An error occured while performing a file operation on a crash

diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm
@@ -17,6 +17,7 @@
 #include <errno.h>
 #include <fcntl.h>
 #import <Foundation/Foundation.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <sys/stat.h>
 #include <sys/types.h>
@@ -146,6 +147,17 @@ OperationStatus SkipReportUpload(const UUID& uuid,
   OperationStatus RequestUpload(const UUID& uuid) override;
 
  private:
+  //! \brief Report states for use with LocateCrashReport().
+  //!
+  //! ReportState may be considered to be a bitfield.
+  enum ReportState : uint8_t {
+    kReportStateWrite = 1 << 0,  // in kWriteDirectory
+    kReportStatePending = 1 << 1,  // in kUploadPendingDirectory
+    kReportStateCompleted = 1 << 2,  // in kCompletedDirectory
+    kReportStateAny =
+        kReportStateWrite | kReportStatePending | kReportStateCompleted,
+  };
+
   //! \brief A private extension of the Report class that maintains bookkeeping
   //!    information of the database.
   struct UploadReport : public Report {
@@ -157,10 +169,12 @@ OperationStatus SkipReportUpload(const UUID& uuid,
   //! \brief Locates a crash report in the database by UUID.
   //!
   //! \param[in] uuid The UUID of the crash report to locate.
+  //! \param[in] desired_state The state of the report to locate, composed of
+  //!     ReportState values.
   //!
   //! \return The full path to the report file, or an empty path if it cannot be
   //!     found.
-  base::FilePath LocateCrashReport(const UUID& uuid);
+  base::FilePath LocateCrashReport(const UUID& uuid, uint8_t desired_state);
 
   //! \brief Obtains an exclusive advisory lock on a file.
   //!
@@ -392,7 +406,7 @@ OperationStatus ReportsInDirectory(const base::FilePath& path,
                                           CrashReportDatabase::Report* report) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
-  base::FilePath path = LocateCrashReport(uuid);
+  base::FilePath path = LocateCrashReport(uuid, kReportStateAny);
   if (path.empty())
     return kReportNotFound;
 
@@ -429,7 +443,7 @@ OperationStatus ReportsInDirectory(const base::FilePath& path,
                                               const Report** report) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
-  base::FilePath report_path = LocateCrashReport(uuid);
+  base::FilePath report_path = LocateCrashReport(uuid, kReportStatePending);
   if (report_path.empty())
     return kReportNotFound;
 
@@ -459,7 +473,8 @@ OperationStatus ReportsInDirectory(const base::FilePath& path,
   DCHECK(report);
   DCHECK(successful || id.empty());
 
-  base::FilePath report_path = LocateCrashReport(report->uuid);
+  base::FilePath report_path =
+      LocateCrashReport(report->uuid, kReportStatePending);
   if (report_path.empty())
     return kReportNotFound;
 
@@ -513,7 +528,7 @@ OperationStatus ReportsInDirectory(const base::FilePath& path,
 
   Metrics::CrashUploadSkipped(reason);
 
-  base::FilePath report_path = LocateCrashReport(uuid);
+  base::FilePath report_path = LocateCrashReport(uuid, kReportStatePending);
   if (report_path.empty())
     return kReportNotFound;
 
@@ -528,7 +543,7 @@ OperationStatus ReportsInDirectory(const base::FilePath& path,
     const UUID& uuid) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
-  base::FilePath report_path = LocateCrashReport(uuid);
+  base::FilePath report_path = LocateCrashReport(uuid, kReportStateAny);
   if (report_path.empty())
     return kReportNotFound;
 
@@ -544,11 +559,25 @@ OperationStatus ReportsInDirectory(const base::FilePath& path,
   return kNoError;
 }
 
-base::FilePath CrashReportDatabaseMac::LocateCrashReport(const UUID& uuid) {
+base::FilePath CrashReportDatabaseMac::LocateCrashReport(
+    const UUID& uuid,
+    uint8_t desired_state) {
   const std::string target_uuid = uuid.ToString();
-  for (size_t i = 0; i < arraysize(kReportDirectories); ++i) {
+
+  std::vector<std::string> report_directories;
+  if (desired_state & kReportStateWrite) {
+    report_directories.push_back(kWriteDirectory);
+  }
+  if (desired_state & kReportStatePending) {
+    report_directories.push_back(kUploadPendingDirectory);
+  }
+  if (desired_state & kReportStateCompleted) {
+    report_directories.push_back(kCompletedDirectory);
+  }
+
+  for (const std::string& report_directory : report_directories) {
     base::FilePath path =
-        base_dir_.Append(kReportDirectories[i])
+        base_dir_.Append(report_directory)
                  .Append(target_uuid + "." + kCrashReportFileExtension);
 
     // Test if the path exists.
@@ -573,7 +602,8 @@ OperationStatus ReportsInDirectory(const base::FilePath& path,
     const UUID& uuid) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
-  base::FilePath report_path = LocateCrashReport(uuid);
+  base::FilePath report_path =
+      LocateCrashReport(uuid, kReportStatePending | kReportStateCompleted);
   if (report_path.empty())
     return kReportNotFound;
 

diff --git a/client/crash_report_database_test.cc b/client/crash_report_database_test.cc
@@ -478,6 +478,22 @@ TEST_F(CrashReportDatabaseTest, DuelingUploads) {
             CrashReportDatabase::kNoError);
 }
 
+TEST_F(CrashReportDatabaseTest, UploadAlreadyUploaded) {
+  CrashReportDatabase::Report report;
+  CreateCrashReport(&report);
+
+  const CrashReportDatabase::Report* upload_report;
+  EXPECT_EQ(db()->GetReportForUploading(report.uuid, &upload_report),
+            CrashReportDatabase::kNoError);
+  EXPECT_EQ(db()->RecordUploadAttempt(upload_report, true, std::string()),
+            CrashReportDatabase::kNoError);
+
+  const CrashReportDatabase::Report* upload_report_2 = nullptr;
+  EXPECT_EQ(db()->GetReportForUploading(report.uuid, &upload_report_2),
+            CrashReportDatabase::kReportNotFound);
+  EXPECT_FALSE(upload_report_2);
+}
+
 TEST_F(CrashReportDatabaseTest, MoveDatabase) {
   CrashReportDatabase::NewReport* new_report;
   EXPECT_EQ(db()->PrepareNewCrashReport(&new_report),

diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc
@@ -242,8 +242,9 @@ class Metadata {
   //! written to disk via Write().
   //!
   //! \return #kNoError on success. #kReportNotFound if there was no report with
-  //!     the specified UUID. #kBusyError if the report was not in the specified
-  //!     state.
+  //!     the specified UUID, or if the report was not in the specified state
+  //!     and was not uploading. #kBusyError if the report was not in the
+  //!     specified state and was uploading.
   OperationStatus FindSingleReportAndMarkDirty(const UUID& uuid,
                                                ReportState desired_state,
                                                ReportDisk** report_disk);
@@ -530,9 +531,13 @@ OperationStatus Metadata::VerifyReportAnyState(const ReportDisk& report_disk) {
 // static
 OperationStatus Metadata::VerifyReport(const ReportDisk& report_disk,
                                        ReportState desired_state) {
-  return (report_disk.state == desired_state)
-             ? VerifyReportAnyState(report_disk)
-             : CrashReportDatabase::kBusyError;
+  if (report_disk.state == desired_state) {
+    return VerifyReportAnyState(report_disk);
+  }
+
+  return report_disk.state == ReportState::kUploading
+             ? CrashReportDatabase::kBusyError
+             : CrashReportDatabase::kReportNotFound;
 }
 
 bool EnsureDirectory(const base::FilePath& path) {
@@ -876,7 +881,7 @@ OperationStatus CrashReportDatabaseWin::RequestUpload(const UUID& uuid) {
   // TODO(gayane): Search for the report only once regardless of its state.
   OperationStatus os = metadata->FindSingleReportAndMarkDirty(
       uuid, ReportState::kCompleted, &report_disk);
-  if (os == kBusyError) {
+  if (os == kReportNotFound) {
     os = metadata->FindSingleReportAndMarkDirty(
         uuid, ReportState::kPending, &report_disk);
   }

diff --git a/handler/crash_report_upload_thread.cc b/handler/crash_report_upload_thread.cc
@@ -252,9 +252,12 @@ void CrashReportUploadThread::ProcessPendingReport(
       break;
 
     case CrashReportDatabase::kBusyError:
+    case CrashReportDatabase::kReportNotFound:
+      // Someone else may have gotten to it first. If they’re working on it now,
+      // this will be kBusyError. If they’ve already finished with it, it’ll be
+      // kReportNotFound.
       return;
 
-    case CrashReportDatabase::kReportNotFound:
     case CrashReportDatabase::kFileSystemError:
     case CrashReportDatabase::kDatabaseError:
       // In these cases, SkipReportUpload() might not work either, but it’s best