Add support for attachments to Windows crash report database

client/crash_report_database.h

@@ -117,7 +117,7 @@ class CrashReportDatabase {
 
     //! \brief Adds an attachment to the report.
     //!
-    //! \note This function is not yet implemented on macOS or Windows.
+    //! \note This function is not yet implemented on macOS.
     //!
     //! \param[in] name The key and name for the attachment, which will be
     //!     included in the http upload. The attachment will not appear in the

client/crash_report_database_win.cc

@@ -29,6 +29,8 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "client/settings.h"
+#include "util/file/directory_reader.h"
+#include "util/file/filesystem.h"
 #include "util/misc/implicit_cast.h"
 #include "util/misc/initialization_state_dcheck.h"
 #include "util/misc/metrics.h"
@@ -37,8 +39,23 @@ namespace crashpad {
 
 namespace {
 
+UUID UUIDFromReportPath(const base::FilePath& path) {
+  UUID uuid;
+  uuid.InitializeFromString(path.RemoveFinalExtension().BaseName().value());
+  return uuid;
+}
+
+bool AttachmentNameIsOK(const std::string& name) {
+  for (const char c : name) {
+    if (c != '_' && c != '-' && c != '.' && !isalnum(c))
+      return false;
+  }
+  return true;
+}
+
 constexpr wchar_t kReportsDirectory[] = L"reports";
 constexpr wchar_t kMetadataFileName[] = L"metadata";
+constexpr wchar_t kAttachmentsDirectory[] = L"attachments";
 
 constexpr wchar_t kSettings[] = L"settings.dat";
 
@@ -599,6 +616,10 @@ class CrashReportDatabaseWin : public CrashReportDatabase {
                                    Metrics::CrashSkippedReason reason) override;
   OperationStatus DeleteReport(const UUID& uuid) override;
   OperationStatus RequestUpload(const UUID& uuid) override;
+  int CleanDatabase(time_t lockfile_ttl) override;
+
+  // Build a filepath for the directory for the report to hold attachments.
+  base::FilePath AttachmentsPath(const UUID& uuid);
 
  private:
   // CrashReportDatabase:
@@ -608,6 +629,15 @@ class CrashReportDatabaseWin : public CrashReportDatabase {
 
   std::unique_ptr<Metadata> AcquireMetadata();
 
+  //! \brief Cleans any attachments that have no associated report.
+  void CleanOrphanedAttachments();
+
+  //! \brief Attempt to remove any attachments associated with the given report UUID.
+  //!     There may not be any, so failing is not an error.
+  //!
+  //! \param[in] uuid The report identifier which attachments to remove.
+  void RemoveAttachmentsByUUID(const UUID& uuid);
+
   base::FilePath base_dir_;
   Settings settings_;
   InitializationStateDcheck initialized_;
@@ -617,12 +647,55 @@ class CrashReportDatabaseWin : public CrashReportDatabase {
 
 FileWriter* CrashReportDatabase::NewReport::AddAttachment(
     const std::string& name) {
-  // Attachments aren't implemented in the Windows database yet.
-  return nullptr;
+  if (!AttachmentNameIsOK(name)) {
+    LOG(ERROR) << "invalid name for attachment " << name;
+    return nullptr;
+  }
+
+  base::FilePath attachments_dir =
+      static_cast<CrashReportDatabaseWin*>(database_)->AttachmentsPath(
+          uuid_);
+  if (!LoggingCreateDirectory(
+          attachments_dir, FilePermissions::kOwnerOnly, true)) {
+    return nullptr;
+  }
+
+  base::FilePath path = attachments_dir.Append(base::UTF8ToUTF16(name));
+
+  auto writer = std::make_unique<FileWriter>();
+  if (!writer->Open(
+          path, FileWriteMode::kCreateOrFail, FilePermissions::kOwnerOnly)) {
+    LOG(ERROR) << "could not open " << base::UTF16ToUTF8(path.value());
+    return nullptr;
+  }
+  attachment_writers_.emplace_back(std::move(writer));
+  attachment_removers_.emplace_back(ScopedRemoveFile(path));
+  return attachment_writers_.back().get();
 }
 
 void CrashReportDatabase::UploadReport::InitializeAttachments() {
-  // Attachments aren't implemented in the Windows database yet.
+  base::FilePath attachments_dir =
+      static_cast<CrashReportDatabaseWin*>(database_)->AttachmentsPath(
+          uuid);
+  DirectoryReader reader;
+  if (!reader.Open(attachments_dir)) {
+    return;
+  }
+
+  base::FilePath filename;
+  DirectoryReader::Result dir_result;
+  while ((dir_result = reader.NextFile(&filename)) ==
+         DirectoryReader::Result::kSuccess) {
+    const base::FilePath filepath(attachments_dir.Append(filename));
+    std::unique_ptr<FileReader> reader(std::make_unique<FileReader>());
+    if (!reader->Open(filepath)) {
+      LOG(ERROR) << "attachment " << base::UTF16ToUTF8(filepath.value())
+                 << " couldn't be opened, skipping";
+      continue;
+    }
+    attachment_readers_.emplace_back(std::move(reader));
+    attachment_map_[base::UTF16ToUTF8(filename.value())] = attachment_readers_.back().get();
+  }
 }
 
 CrashReportDatabaseWin::CrashReportDatabaseWin(const base::FilePath& path)
@@ -646,6 +719,9 @@ bool CrashReportDatabaseWin::Initialize(bool may_create) {
   if (!CreateDirectoryIfNecessary(base_dir_.Append(kReportsDirectory)))
     return false;
 
+  if (!CreateDirectoryIfNecessary(base_dir_.Append(kAttachmentsDirectory)))
+    return false;
+
   if (!settings_.Initialize(base_dir_.Append(kSettings)))
     return false;
 
@@ -688,6 +764,14 @@ OperationStatus CrashReportDatabaseWin::FinishedWritingCrashReport(
 
   ignore_result(report->file_remover_.release());
 
+  // Close all the attachments and disarm their removers too.
+  for (auto& writer : report->attachment_writers_) {
+    writer->Close();
+  }
+  for (auto& remover : report->attachment_removers_) {
+    ignore_result(remover.release());
+  }
+
   *uuid = report->ReportID();
 
   Metrics::CrashReportPending(Metrics::PendingReportReason::kNewlyCreated);
@@ -705,9 +789,11 @@ OperationStatus CrashReportDatabaseWin::LookUpCrashReport(const UUID& uuid,
     return kDatabaseError;
   // Find and return a copy of the matching report.
   const ReportDisk* report_disk;
-  OperationStatus os = metadata->FindSingleReport(uuid, &report_disk);
+  const OperationStatus os = metadata->FindSingleReport(uuid, &report_disk);
   if (os == kNoError)
     *report = *report_disk;
+  if (os == kReportNotFound)
+    RemoveAttachmentsByUUID(uuid);
   return os;
 }
 
@@ -805,11 +891,14 @@ OperationStatus CrashReportDatabaseWin::DeleteReport(const UUID& uuid) {
   if (os != kNoError)
     return os;
 
+  RemoveAttachmentsByUUID(uuid);
+
   if (!DeleteFile(report_path.value().c_str())) {
     PLOG(ERROR) << "DeleteFile "
                 << base::UTF16ToUTF8(report_path.value());
     return kFileSystemError;
   }
+
   return kNoError;
 }
 
@@ -835,7 +924,65 @@ OperationStatus CrashReportDatabaseWin::SkipReportUpload(
 
 std::unique_ptr<Metadata> CrashReportDatabaseWin::AcquireMetadata() {
   base::FilePath metadata_file = base_dir_.Append(kMetadataFileName);
-  return Metadata::Create(metadata_file, base_dir_.Append(kReportsDirectory));
+  return Metadata::Create(metadata_file,
+                          base_dir_.Append(kReportsDirectory));
+}
+
+void CrashReportDatabaseWin::CleanOrphanedAttachments()
+{
+    base::FilePath root_attachments_dir(base_dir_.Append(kAttachmentsDirectory));
+    DirectoryReader reader;
+    if (!reader.Open(root_attachments_dir)) {
+      LOG(ERROR) << "no attachments dir";
+      return;
+    }
+
+    std::unique_ptr<Metadata> metadata(AcquireMetadata());
+    if (!metadata)
+        return;
+
+    base::FilePath filename;
+    DirectoryReader::Result result;
+    while ((result = reader.NextFile(&filename)) ==
+           DirectoryReader::Result::kSuccess) {
+      const base::FilePath path(root_attachments_dir.Append(filename));
+      if (IsDirectory(path, false)) {
+        UUID uuid;
+        if (!uuid.InitializeFromString(filename.value())) {
+          LOG(ERROR) << "unexpected attachment dir name " << base::UTF16ToUTF8(filename.value());
+          continue;
+        }
+
+        // Check to see if the report exist.
+        const ReportDisk* report_disk;
+        const OperationStatus os = metadata->FindSingleReport(uuid, &report_disk);
+        if (os != OperationStatus::kReportNotFound) {
+          continue;
+        }
+
+        // Couldn't find a report, assume these attachments are orphaned.
+        RemoveAttachmentsByUUID(uuid);
+      }
+    }
+}
+
+void CrashReportDatabaseWin::RemoveAttachmentsByUUID(const UUID &uuid)
+{
+    base::FilePath attachments_dir = AttachmentsPath(uuid);
+    DirectoryReader reader;
+    if (!reader.Open(attachments_dir)) {
+      return;
+    }
+
+    base::FilePath filename;
+    DirectoryReader::Result result;
+    while ((result = reader.NextFile(&filename)) ==
+           DirectoryReader::Result::kSuccess) {
+      const base::FilePath filepath(attachments_dir.Append(filename));
+      LoggingRemoveFile(filepath);
+    }
+
+    LoggingRemoveDirectory(attachments_dir);
 }
 
 std::unique_ptr<CrashReportDatabase> InitializeInternal(
@@ -881,6 +1028,16 @@ OperationStatus CrashReportDatabaseWin::RequestUpload(const UUID& uuid) {
   return kNoError;
 }
 
+int CrashReportDatabaseWin::CleanDatabase(time_t lockfile_ttl) {
+  (void)lockfile_ttl;
+  CleanOrphanedAttachments();
+  return 0;
+}
+
+base::FilePath CrashReportDatabaseWin::AttachmentsPath(const UUID& uuid) {
+  return base_dir_.Append(kAttachmentsDirectory).Append(uuid.ToString16());
+}
+
 // static
 std::unique_ptr<CrashReportDatabase> CrashReportDatabase::Initialize(
     const base::FilePath& path) {

handler/win/crash_report_exception_handler.cc

@@ -35,10 +35,12 @@ CrashReportExceptionHandler::CrashReportExceptionHandler(
     CrashReportDatabase* database,
     CrashReportUploadThread* upload_thread,
     const std::map<std::string, std::string>* process_annotations,
+    const std::map<std::string, base::FilePath>* process_attachments,
     const UserStreamDataSources* user_stream_data_sources)
     : database_(database),
       upload_thread_(upload_thread),
       process_annotations_(process_annotations),
+      process_attachments_(process_attachments),
       user_stream_data_sources_(user_stream_data_sources) {}
 
 CrashReportExceptionHandler::~CrashReportExceptionHandler() {
@@ -114,6 +116,24 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException(
       return termination_code;
     }
 
+    if (process_attachments_) {
+      // Note that attachments are read at this point each time rather than once
+      // so that if the contents of the file has changed it will be re-read for
+      // each upload (e.g. in the case of a log file).
+      for (const auto& it : *process_attachments_) {
+        FileWriter* writer = new_report->AddAttachment(it.first);
+        if (writer) {
+          std::string contents;
+          if (!LoggingReadEntireFile(it.second, &contents)) {
+            // Not being able to read the file isn't considered fatal, and
+            // should not prevent the report from being processed.
+            continue;
+          }
+          writer->Write(contents.data(), contents.size());
+        }
+      }
+    }
+
     UUID uuid;
     database_status =
         database_->FinishedWritingCrashReport(std::move(new_report), &uuid);

handler/win/crash_report_exception_handler.h

@@ -20,6 +20,7 @@
 #include <map>
 #include <string>
 
+#include "base/files/file_path.h"
 #include "base/macros.h"
 #include "handler/user_stream_data_source.h"
 #include "util/win/exception_handler_server.h"
@@ -49,6 +50,10 @@ class CrashReportExceptionHandler : public ExceptionHandlerServer::Delegate {
   //!     To interoperate with Breakpad servers, the recommended practice is to
   //!     specify values for the `"prod"` and `"ver"` keys as process
   //!     annotations.
+  //! \param[in] process_attachments A map of file name keys to file paths to be
+  //!     included in the report. Each time a report is written, the file paths
+  //!     will be read in their entirety and included in the report using the
+  //!     file name key as the name in the http upload.
   //! \param[in] user_stream_data_sources Data sources to be used to extend
   //!     crash reports. For each crash report that is written, the data sources
   //!     are called in turn. These data sources may contribute additional
@@ -57,6 +62,7 @@ class CrashReportExceptionHandler : public ExceptionHandlerServer::Delegate {
       CrashReportDatabase* database,
       CrashReportUploadThread* upload_thread,
       const std::map<std::string, std::string>* process_annotations,
+      const std::map<std::string, base::FilePath>* process_attachments,
       const UserStreamDataSources* user_stream_data_sources);
 
   ~CrashReportExceptionHandler();
@@ -75,6 +81,7 @@ class CrashReportExceptionHandler : public ExceptionHandlerServer::Delegate {
   CrashReportDatabase* database_;  // weak
   CrashReportUploadThread* upload_thread_;  // weak
   const std::map<std::string, std::string>* process_annotations_;  // weak
+  const std::map<std::string, base::FilePath>* process_attachments_; // weak
   const UserStreamDataSources* user_stream_data_sources_;  // weak
 
   DISALLOW_COPY_AND_ASSIGN(CrashReportExceptionHandler);