crashpad_database_util: Don’t create a database unless explicitly asked

I’ve accidentally created Crashpad databases when running crashpad_database_util by mistyping the argument to --database. Typical users of crashpad_database_util probably don’t want the database to be created. This adds a new --create option to crashpad_database_util that is required to get it to create a database. If not present, a database will not be created if it does not already exist. TEST=crashpad_client_test CrashReportDatabaseTest.* R=rsesek@chromium.org, scottmg@chromium.org Review URL: https://codereview.chromium.org/1395653002 .
2024-12-26 23:01:05 +08:00 · 2015-10-08 13:10:02 -04:00 · 2015-10-08 13:10:02 -04:00 · 553a643475
commit 553a643475
parent efe97b8600
7 changed files with 172 additions and 87 deletions
--- a/client/crash_report_database.h
+++ b/client/crash_report_database.h
@ -165,14 +165,34 @@ class CrashReportDatabase {

  virtual ~CrashReportDatabase() {}

-  //! \brief Initializes a database of crash reports.
+  //! \brief Opens a database of crash reports, possibly creating it.
  //!
-  //! \param[in] path A path to the database to be created or opened.
+  //! \param[in] path A path to the database to be created or opened. If the
+  //!     database does not yet exist, it will be created if possible. Note that
+  //!     for databases implemented as directory structures, existence refers
+  //!     solely to the outermost directory.
  //!
  //! \return A database object on success, `nullptr` on failure with an error
  //!     logged.
+  //!
+  //! \sa InitializeWithoutCreating
  static scoped_ptr<CrashReportDatabase> Initialize(const base::FilePath& path);

+  //! \brief Opens an existing database of crash reports.
+  //!
+  //! \param[in] path A path to the database to be opened. If the database does
+  //!     not yet exist, it will not be created. Note that for databases
+  //!     implemented as directory structures, existence refers solely to the
+  //!     outermost directory. On such databases, as long as the outermost
+  //!     directory is present, this method will create the inner structure.
+  //!
+  //! \return A database object on success, `nullptr` on failure with an error
+  //!     logged.
+  //!
+  //! \sa Initialize
+  static scoped_ptr<CrashReportDatabase> InitializeWithoutCreating(
+      const base::FilePath& path);
+
  //! \brief Returns the Settings object for this database.
  //!
  //! \return A weak pointer to the Settings object, which is owned by the
--- a/client/crash_report_database_mac.mm
+++ b/client/crash_report_database_mac.mm
@ -63,29 +63,35 @@ const char kXattrUploadAttemptCount[] = "upload_count";

 const char kXattrDatabaseInitialized[] = "initialized";

+// Ensures that the node at |path| is a directory. If the |path| refers to a
+// file, rather than a directory, returns false. Otherwise, returns true,
+// indicating that |path| already was a directory.
+bool EnsureDirectoryExists(const base::FilePath& path) {
+  struct stat st;
+  if (stat(path.value().c_str(), &st) != 0) {
+    PLOG(ERROR) << "stat " << path.value();
+    return false;
+  }
+  if (!S_ISDIR(st.st_mode)) {
+    LOG(ERROR) << "stat " << path.value() << ": not a directory";
+    return false;
+  }
+  return true;
+}
+
 // Ensures that the node at |path| is a directory, and creates it if it does
-// not exist. If the |path| points to a file, rather than a directory, or the
+// not exist. If the |path| refers to a file, rather than a directory, or the
 // directory could not be created, returns false. Otherwise, returns true,
 // indicating that |path| already was or now is a directory.
 bool CreateOrEnsureDirectoryExists(const base::FilePath& path) {
  if (mkdir(path.value().c_str(), 0755) == 0) {
    return true;
-  } else if (errno == EEXIST) {
-    struct stat st;
-    if (stat(path.value().c_str(), &st) != 0) {
-      PLOG(ERROR) << "stat";
-      return false;
-    }
-    if (S_ISDIR(st.st_mode)) {
-      return true;
-    } else {
-      LOG(ERROR) << "not a directory";
-      return false;
-    }
-  } else {
-    PLOG(ERROR) << "mkdir";
+  }
+  if (errno != EEXIST) {
+    PLOG(ERROR) << "mkdir " << path.value();
    return false;
  }
+  return EnsureDirectoryExists(path);
 }

 //! \brief A CrashReportDatabase that uses HFS+ extended attributes to store
@ -107,7 +113,7 @@ class CrashReportDatabaseMac : public CrashReportDatabase {
  explicit CrashReportDatabaseMac(const base::FilePath& path);
  virtual ~CrashReportDatabaseMac();

-  bool Initialize();
+  bool Initialize(bool may_create);

  // CrashReportDatabase:
  Settings* GetSettings() override;
@ -115,8 +121,7 @@ class CrashReportDatabaseMac : public CrashReportDatabase {
  OperationStatus FinishedWritingCrashReport(NewReport* report,
                                             UUID* uuid) override;
  OperationStatus ErrorWritingCrashReport(NewReport* report) override;
-  OperationStatus LookUpCrashReport(const UUID& uuid,
-                                    Report* report) override;
+  OperationStatus LookUpCrashReport(const UUID& uuid, Report* report) override;
  OperationStatus GetPendingReports(std::vector<Report>* reports) override;
  OperationStatus GetCompletedReports(std::vector<Report>* reports) override;
  OperationStatus GetReportForUploading(const UUID& uuid,
@ -181,7 +186,6 @@ class CrashReportDatabaseMac : public CrashReportDatabase {
  static OperationStatus ReportsInDirectory(const base::FilePath& path,
                                            std::vector<Report>* reports);

-
  //! \brief Creates a database xattr name from the short constant name.
  //!
  //! \param[in] name The short name of the extended attribute.
@ -205,12 +209,17 @@ CrashReportDatabaseMac::CrashReportDatabaseMac(const base::FilePath& path)

 CrashReportDatabaseMac::~CrashReportDatabaseMac() {}

-bool CrashReportDatabaseMac::Initialize() {
+bool CrashReportDatabaseMac::Initialize(bool may_create) {
  INITIALIZATION_STATE_SET_INITIALIZING(initialized_);

  // Check if the database already exists.
-  if (!CreateOrEnsureDirectoryExists(base_dir_))
+  if (may_create) {
+    if (!CreateOrEnsureDirectoryExists(base_dir_)) {
      return false;
+    }
+  } else if (!EnsureDirectoryExists(base_dir_)) {
+    return false;
+  }

  // Create the three processing directories for the database.
  for (size_t i = 0; i < arraysize(kReportDirectories); ++i) {
@ -616,17 +625,28 @@ std::string CrashReportDatabaseMac::XattrName(const base::StringPiece& name) {
  return base::StringPrintf("com.googlecode.crashpad.%s", name.data());
 }

-}  // namespace
-
-// static
-scoped_ptr<CrashReportDatabase> CrashReportDatabase::Initialize(
-    const base::FilePath& path) {
+scoped_ptr<CrashReportDatabase> InitializeInternal(const base::FilePath& path,
+                                                   bool may_create) {
  scoped_ptr<CrashReportDatabaseMac> database_mac(
      new CrashReportDatabaseMac(path));
-  if (!database_mac->Initialize())
+  if (!database_mac->Initialize(may_create))
    database_mac.reset();

  return scoped_ptr<CrashReportDatabase>(database_mac.release());
 }

+}  // namespace
+
+// static
+scoped_ptr<CrashReportDatabase> CrashReportDatabase::Initialize(
+    const base::FilePath& path) {
+  return InitializeInternal(path, true);
+}
+
+// static
+scoped_ptr<CrashReportDatabase> CrashReportDatabase::InitializeWithoutCreating(
+    const base::FilePath& path) {
+  return InitializeInternal(path, false);
+}
+
 }  // namespace crashpad
--- a/client/crash_report_database_test.cc
+++ b/client/crash_report_database_test.cc
@ -75,8 +75,7 @@ class CrashReportDatabaseTest : public testing::Test {
              db_->GetReportForUploading(uuid, &report));
    EXPECT_NE(UUID(), report->uuid);
    EXPECT_FALSE(report->file_path.empty());
-    EXPECT_TRUE(FileExists(report->file_path))
-        << report->file_path.value();
+    EXPECT_TRUE(FileExists(report->file_path)) << report->file_path.value();
    EXPECT_GT(report->creation_time, 0);
    EXPECT_EQ(CrashReportDatabase::kNoError,
              db_->RecordUploadAttempt(report, successful, id));
@ -116,7 +115,7 @@ TEST_F(CrashReportDatabaseTest, Initialize) {

  Settings* settings = db()->GetSettings();

-  UUID client_ids[2];
+  UUID client_ids[3];
  ASSERT_TRUE(settings->GetClientID(&client_ids[0]));
  EXPECT_NE(client_ids[0], UUID());

@ -127,7 +126,7 @@ TEST_F(CrashReportDatabaseTest, Initialize) {
  // Close and reopen the database at the same path.
  ResetDatabase();
  EXPECT_FALSE(db());
-  auto db = CrashReportDatabase::Initialize(path());
+  auto db = CrashReportDatabase::InitializeWithoutCreating(path());
  ASSERT_TRUE(db);

  settings = db->GetSettings();
@ -138,12 +137,31 @@ TEST_F(CrashReportDatabaseTest, Initialize) {
  ASSERT_TRUE(settings->GetLastUploadAttemptTime(&last_upload_attempt_time));
  EXPECT_EQ(0, last_upload_attempt_time);

+  // Check that the database can also be opened by the method that is permitted
+  // to create it.
+  db = CrashReportDatabase::Initialize(path());
+  ASSERT_TRUE(db);
+
+  settings = db->GetSettings();
+
+  ASSERT_TRUE(settings->GetClientID(&client_ids[2]));
+  EXPECT_EQ(client_ids[0], client_ids[2]);
+
+  ASSERT_TRUE(settings->GetLastUploadAttemptTime(&last_upload_attempt_time));
+  EXPECT_EQ(0, last_upload_attempt_time);
+
  std::vector<CrashReportDatabase::Report> reports;
  EXPECT_EQ(CrashReportDatabase::kNoError, db->GetPendingReports(&reports));
  EXPECT_TRUE(reports.empty());
  reports.clear();
  EXPECT_EQ(CrashReportDatabase::kNoError, db->GetCompletedReports(&reports));
  EXPECT_TRUE(reports.empty());
+
+  // InitializeWithoutCreating() shouldn’t create a nonexistent database.
+  base::FilePath non_database_path =
+      path().DirName().Append(FILE_PATH_LITERAL("not_a_database"));
+  db = CrashReportDatabase::InitializeWithoutCreating(non_database_path);
+  EXPECT_FALSE(db);
 }

 TEST_F(CrashReportDatabaseTest, NewCrashReport) {
@ -163,14 +181,12 @@ TEST_F(CrashReportDatabaseTest, NewCrashReport) {
  ExpectPreparedCrashReport(report);

  std::vector<CrashReportDatabase::Report> reports;
-  EXPECT_EQ(CrashReportDatabase::kNoError,
-            db()->GetPendingReports(&reports));
+  EXPECT_EQ(CrashReportDatabase::kNoError, db()->GetPendingReports(&reports));
  ASSERT_EQ(1u, reports.size());
  EXPECT_EQ(report.uuid, reports[0].uuid);

  reports.clear();
-  EXPECT_EQ(CrashReportDatabase::kNoError,
-            db()->GetCompletedReports(&reports));
+  EXPECT_EQ(CrashReportDatabase::kNoError, db()->GetCompletedReports(&reports));
  EXPECT_TRUE(reports.empty());
 }

@ -320,8 +336,7 @@ TEST_F(CrashReportDatabaseTest, GetCompletedAndNotUploadedReports) {
  const UUID& report_4_uuid = reports[4].uuid;

  std::vector<CrashReportDatabase::Report> pending;
-  EXPECT_EQ(CrashReportDatabase::kNoError,
-            db()->GetPendingReports(&pending));
+  EXPECT_EQ(CrashReportDatabase::kNoError, db()->GetPendingReports(&pending));

  std::vector<CrashReportDatabase::Report> completed;
  EXPECT_EQ(CrashReportDatabase::kNoError,
@ -334,8 +349,7 @@ TEST_F(CrashReportDatabaseTest, GetCompletedAndNotUploadedReports) {
  UploadReport(report_1_uuid, true, "report1");

  pending.clear();
-  EXPECT_EQ(CrashReportDatabase::kNoError,
-            db()->GetPendingReports(&pending));
+  EXPECT_EQ(CrashReportDatabase::kNoError, db()->GetPendingReports(&pending));
  completed.clear();
  EXPECT_EQ(CrashReportDatabase::kNoError,
            db()->GetCompletedReports(&completed));
@ -357,8 +371,7 @@ TEST_F(CrashReportDatabaseTest, GetCompletedAndNotUploadedReports) {
  UploadReport(report_2_uuid, false, std::string());

  pending.clear();
-  EXPECT_EQ(CrashReportDatabase::kNoError,
-            db()->GetPendingReports(&pending));
+  EXPECT_EQ(CrashReportDatabase::kNoError, db()->GetPendingReports(&pending));
  completed.clear();
  EXPECT_EQ(CrashReportDatabase::kNoError,
            db()->GetCompletedReports(&completed));
@ -380,8 +393,7 @@ TEST_F(CrashReportDatabaseTest, GetCompletedAndNotUploadedReports) {
  UploadReport(report_4_uuid, true, "report_4");

  pending.clear();
-  EXPECT_EQ(CrashReportDatabase::kNoError,
-            db()->GetPendingReports(&pending));
+  EXPECT_EQ(CrashReportDatabase::kNoError, db()->GetPendingReports(&pending));
  completed.clear();
  EXPECT_EQ(CrashReportDatabase::kNoError,
            db()->GetCompletedReports(&completed));
@ -393,8 +405,7 @@ TEST_F(CrashReportDatabaseTest, GetCompletedAndNotUploadedReports) {
  UploadReport(report_2_uuid, true, "report 2");

  pending.clear();
-  EXPECT_EQ(CrashReportDatabase::kNoError,
-            db()->GetPendingReports(&pending));
+  EXPECT_EQ(CrashReportDatabase::kNoError, db()->GetPendingReports(&pending));
  completed.clear();
  EXPECT_EQ(CrashReportDatabase::kNoError,
            db()->GetCompletedReports(&completed));
@ -403,8 +414,7 @@ TEST_F(CrashReportDatabaseTest, GetCompletedAndNotUploadedReports) {
  ASSERT_EQ(3u, completed.size());

  for (const auto& report : pending) {
-    EXPECT_TRUE(report.uuid == report_0_uuid ||
-                report.uuid == report_3_uuid);
+    EXPECT_TRUE(report.uuid == report_0_uuid || report.uuid == report_3_uuid);
    EXPECT_FALSE(report.file_path.empty());
  }

@ -413,8 +423,7 @@ TEST_F(CrashReportDatabaseTest, GetCompletedAndNotUploadedReports) {
            db()->SkipReportUpload(report_3_uuid));

  pending.clear();
-  EXPECT_EQ(CrashReportDatabase::kNoError,
-            db()->GetPendingReports(&pending));
+  EXPECT_EQ(CrashReportDatabase::kNoError, db()->GetPendingReports(&pending));
  completed.clear();
  EXPECT_EQ(CrashReportDatabase::kNoError,
            db()->GetCompletedReports(&completed));
--- a/client/crash_report_database_win.cc
+++ b/client/crash_report_database_win.cc
@ -510,6 +510,21 @@ OperationStatus Metadata::VerifyReport(const ReportDisk& report_disk,
             : CrashReportDatabase::kBusyError;
 }

+bool EnsureDirectory(const base::FilePath& path) {
+  DWORD fileattr = GetFileAttributes(path.value().c_str());
+  if (fileattr == INVALID_FILE_ATTRIBUTES) {
+    PLOG(ERROR) << "GetFileAttributes " << base::UTF16ToUTF8(path.value());
+    return false;
+  }
+  if ((fileattr & FILE_ATTRIBUTE_DIRECTORY) == 0) {
+    LOG(ERROR) << "GetFileAttributes "
+               << base::UTF16ToUTF8(path.value())
+               << ": not a directory";
+    return false;
+  }
+  return true;
+}
+
 //! \brief Ensures that the node at path is a directory, and creates it if it
 //!     does not exist.
 //!
@ -520,18 +535,10 @@ bool CreateDirectoryIfNecessary(const base::FilePath& path) {
  if (CreateDirectory(path.value().c_str(), nullptr))
    return true;
  if (GetLastError() != ERROR_ALREADY_EXISTS) {
-    PLOG(ERROR) << "CreateDirectory";
+    PLOG(ERROR) << "CreateDirectory " << base::UTF16ToUTF8(path.value());
    return false;
  }
-  DWORD fileattr = GetFileAttributes(path.value().c_str());
-  if (fileattr == INVALID_FILE_ATTRIBUTES) {
-    PLOG(ERROR) << "GetFileAttributes";
-    return false;
-  }
-  if ((fileattr & FILE_ATTRIBUTE_DIRECTORY) != 0)
-    return true;
-  LOG(ERROR) << "not a directory";
-  return false;
+  return EnsureDirectory(path);
 }

 // CrashReportDatabaseWin ------------------------------------------------------
@ -541,7 +548,7 @@ class CrashReportDatabaseWin : public CrashReportDatabase {
  explicit CrashReportDatabaseWin(const base::FilePath& path);
  ~CrashReportDatabaseWin() override;

-  bool Initialize();
+  bool Initialize(bool may_create);

  // CrashReportDatabase:
  Settings* GetSettings() override;
@ -580,12 +587,19 @@ CrashReportDatabaseWin::CrashReportDatabaseWin(const base::FilePath& path)
 CrashReportDatabaseWin::~CrashReportDatabaseWin() {
 }

-bool CrashReportDatabaseWin::Initialize() {
+bool CrashReportDatabaseWin::Initialize(bool may_create) {
  INITIALIZATION_STATE_SET_INITIALIZING(initialized_);

-  // Ensure the database and report subdirectories exist.
-  if (!CreateDirectoryIfNecessary(base_dir_) ||
-      !CreateDirectoryIfNecessary(base_dir_.Append(kReportsDirectory)))
+  // Ensure the database directory exists.
+  if (may_create) {
+    if (!CreateDirectoryIfNecessary(base_dir_))
+      return false;
+  } else if (!EnsureDirectory(base_dir_)) {
+    return false;
+  }
+
+  // Ensure that the report subdirectory exists.
+  if (!CreateDirectoryIfNecessary(base_dir_.Append(kReportsDirectory)))
    return false;

  if (!settings_.Initialize())
@ -796,15 +810,27 @@ scoped_ptr<Metadata> CrashReportDatabaseWin::AcquireMetadata() {
  return Metadata::Create(metadata_file, base_dir_.Append(kReportsDirectory));
 }

+scoped_ptr<CrashReportDatabase> InitializeInternal(
+    const base::FilePath& path, bool may_create) {
+  scoped_ptr<CrashReportDatabaseWin> database_win(
+      new CrashReportDatabaseWin(path));
+  return database_win->Initialize(may_create)
+             ? database_win.Pass()
+             : scoped_ptr<CrashReportDatabaseWin>();
+}
+
 }  // namespace

 // static
 scoped_ptr<CrashReportDatabase> CrashReportDatabase::Initialize(
    const base::FilePath& path) {
-  scoped_ptr<CrashReportDatabaseWin> database_win(
-      new CrashReportDatabaseWin(path));
-  return database_win->Initialize() ? database_win.Pass()
-                                    : scoped_ptr<CrashReportDatabaseWin>();
+  return InitializeInternal(path, true);
+}
+
+// static
+scoped_ptr<CrashReportDatabase> CrashReportDatabase::InitializeWithoutCreating(
+    const base::FilePath& path) {
+  return InitializeInternal(path, false);
 }

 }  // namespace crashpad
--- a/handler/main.cc
+++ b/handler/main.cc
@ -21,7 +21,6 @@
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "client/crash_report_database.h"
 #include "client/crashpad_client.h"
@ -225,14 +224,9 @@ int HandlerMain(int argc, char* argv[]) {
                                MACH_MSG_TYPE_MAKE_SEND);
 #endif  // OS_MACOSX

-  scoped_ptr<CrashReportDatabase> database(
-      CrashReportDatabase::Initialize(base::FilePath(
-#if defined(OS_MACOSX)
-          options.database
-#elif defined(OS_WIN)
-          base::UTF8ToUTF16(options.database)
-#endif
-          )));
+  scoped_ptr<CrashReportDatabase> database(CrashReportDatabase::Initialize(
+      base::FilePath(ToolSupport::CommandLineArgumentToFilePathStringType(
+          options.database))));
  if (!database) {
    return EXIT_FAILURE;
  }
--- a/tools/crashpad_database_util.ad
+++ b/tools/crashpad_database_util.ad
@ -41,10 +41,13 @@ client library. This tool exists to allow developers to manipulate a Crashpad
 database.

 == Options
+*--create*::
+Creates the database identified by *--database* if it does not exist, provided
+that the parent directory of 'PATH' exists.
+
 *-d*, *--database*='PATH'::
 Use 'PATH' as the path to the Crashpad crash report database. This option is
-required. If the database does not exist, it will be created, provided that the
-parent directory of 'PATH' exists.
+required. The database must already exist unless *--create* is also specified.

 *--show-client-id*::
 Show the client ID stored in the database’s settings. The client ID is formatted
--- a/tools/crashpad_database_util.cc
+++ b/tools/crashpad_database_util.cc
@ -46,6 +46,7 @@ void Usage(const base::FilePath& me) {
 "Usage: %" PRFilePath " [OPTION]... PID\n"
 "Operate on Crashpad crash report databases.\n"
 "\n"
+"      --create                    allow database at PATH to be created\n"
 "  -d, --database=PATH             operate on the crash report database at PATH\n"
 "      --show-client-id            show the client ID\n"
 "      --show-uploads-enabled      show whether uploads are enabled\n"
@ -72,6 +73,7 @@ struct Options {
  const char* database;
  const char* set_last_upload_attempt_time_string;
  time_t set_last_upload_attempt_time;
+  bool create;
  bool show_client_id;
  bool show_uploads_enabled;
  bool show_last_upload_attempt_time;
@ -253,6 +255,7 @@ int DatabaseUtilMain(int argc, char* argv[]) {

    // Long options without short equivalents.
    kOptionLastChar = 255,
+    kOptionCreate,
    kOptionShowClientID,
    kOptionShowUploadsEnabled,
    kOptionShowLastUploadAttemptTime,
@ -271,6 +274,7 @@ int DatabaseUtilMain(int argc, char* argv[]) {
  };

  const option long_options[] = {
+      {"create", no_argument, nullptr, kOptionCreate},
      {"database", required_argument, nullptr, kOptionDatabase},
      {"show-client-id", no_argument, nullptr, kOptionShowClientID},
      {"show-uploads-enabled", no_argument, nullptr, kOptionShowUploadsEnabled},
@ -305,6 +309,10 @@ int DatabaseUtilMain(int argc, char* argv[]) {
  int opt;
  while ((opt = getopt_long(argc, argv, "d:", long_options, nullptr)) != -1) {
    switch (opt) {
+      case kOptionCreate: {
+        options.create = true;
+        break;
+      }
      case kOptionDatabase: {
        options.database = optarg;
        break;
@ -410,14 +418,19 @@ int DatabaseUtilMain(int argc, char* argv[]) {
      options.has_set_uploads_enabled +
      (options.set_last_upload_attempt_time_string != nullptr);

-  if (show_operations + set_operations == 0) {
+  if ((options.create ? 1 : 0) + show_operations + set_operations == 0) {
    ToolSupport::UsageHint(me, "nothing to do");
    return EXIT_FAILURE;
  }

-  scoped_ptr<CrashReportDatabase> database(CrashReportDatabase::Initialize(
-      base::FilePath(ToolSupport::CommandLineArgumentToFilePathStringType(
-          options.database))));
+  scoped_ptr<CrashReportDatabase> database;
+  base::FilePath database_path = base::FilePath(
+      ToolSupport::CommandLineArgumentToFilePathStringType(options.database));
+  if (options.create) {
+    database = CrashReportDatabase::Initialize(database_path);
+  } else {
+    database = CrashReportDatabase::InitializeWithoutCreating(database_path);
+  }
  if (!database) {
    return EXIT_FAILURE;
  }