ios: Don't use POSIX file locks for iOS intermediate dumps.

Instead use a custom mechanism based on the filename. Rather than a
filename of <uuid>, instead name the file <bundle-id>|<uuid>[.locked].
A locked file will have the optional .locked extension. Files can be
unlocked after writing an intermediate dump, or during initialization by
looking for matching bundle-ids.

Clients that call ProcessIntermediateDumps() will clean up any leftover
locked intermediate dumps. Clients that never call
ProcessIntermediateDumps, such as extensions that leave this up to the
main application, will be cleaned up in a followup change.

Bug: crashpad:31
Change-Id: Icd4aaa3b79351870fbe9b8463cfbdf7cff7d5f87
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3229429
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>

client/crashpad_client_ios.cc

@@ -68,7 +68,8 @@ class CrashHandler : public Thread,
                   const std::string& url,
                   const std::map<std::string, std::string>& annotations) {
     INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
-    if (!in_process_handler_.Initialize(database, url, annotations) ||
+    if (!in_process_handler_.Initialize(
+            database, url, annotations, system_data_) ||
         !InstallMachExceptionHandler() ||
         !Signals::InstallHandler(SIGABRT, CatchSignal, 0, &old_action_)) {
       LOG(ERROR) << "Unable to initialize Crashpad.";

client/ios_handler/in_process_handler.cc

@@ -36,6 +36,13 @@ void CreateDirectory(const base::FilePath& path) {
   }
 }
 
+// The file extension used to indicate a file is locked.
+constexpr char kLockedExtension[] = ".locked";
+
+// The seperator used to break the bundle id (e.g. com.chromium.ios) from the
+// uuid in the intermediate dump file name.
+constexpr char kBundleSeperator[] = "@";
+
 }  // namespace
 
 namespace crashpad {
@@ -50,10 +57,13 @@ InProcessHandler::~InProcessHandler() {
 bool InProcessHandler::Initialize(
     const base::FilePath& database,
     const std::string& url,
-    const std::map<std::string, std::string>& annotations) {
+    const std::map<std::string, std::string>& annotations,
+    const IOSSystemDataCollector& system_data) {
   INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
   annotations_ = annotations;
   database_ = CrashReportDatabase::Initialize(database);
+  bundle_identifier_and_seperator_ =
+      system_data.BundleIdentifier() + kBundleSeperator;
 
   if (!url.empty()) {
     // TODO(scottmg): options.rate_limit should be removed when we have a
@@ -213,16 +223,22 @@ std::vector<base::FilePath> InProcessHandler::PendingFiles() {
   DirectoryReader::Result result;
   while ((result = reader.NextFile(&file)) ==
          DirectoryReader::Result::kSuccess) {
-    file = base_dir_.Append(file);
-    if (file != current_file_) {
-      ScopedFileHandle fd(LoggingOpenFileForRead(file));
-      if (LoggingLockFile(fd.get(),
-                          FileLocking::kExclusive,
-                          FileLockingBlocking::kNonBlocking) ==
-          FileLockingResult::kSuccess) {
-        files.push_back(file);
-      }
+    // Don't try to process files marked as 'locked' from a different bundle id.
+    if (file.value().compare(0,
+                             bundle_identifier_and_seperator_.size(),
+                             bundle_identifier_and_seperator_) != 0 &&
+        file.FinalExtension() == kLockedExtension) {
+      continue;
     }
+
+    // Never process the current file.
+    file = base_dir_.Append(file);
+    if (file == current_file_)
+      continue;
+
+    // Otherwise, include any other unlocked, or locked files matching
+    // |bundle_identifier_and_seperator_|.
+    files.push_back(file);
   }
   return files;
 }
@@ -272,10 +288,17 @@ InProcessHandler::ScopedReport::ScopedReport(
 }
 
 bool InProcessHandler::OpenNewFile() {
+  if (!current_file_.empty()) {
+    // Remove .lock extension so this dump can be processed on next run by this
+    // client, or a client with a different bundle id that can access this dump.
+    base::FilePath new_path = current_file_.RemoveFinalExtension();
+    MoveFileOrDirectory(current_file_, new_path);
+  }
   UUID uuid;
   uuid.InitializeWithNew();
-  const std::string uuid_string = uuid.ToString();
-  current_file_ = base_dir_.Append(uuid_string);
+  const std::string file_string =
+      bundle_identifier_and_seperator_ + uuid.ToString() + kLockedExtension;
+  current_file_ = base_dir_.Append(file_string);
   writer_ = std::make_unique<IOSIntermediateDumpWriter>();
   if (!writer_->Open(current_file_)) {
     DLOG(ERROR) << "Unable to open intermediate dump file: "

client/ios_handler/in_process_handler.h

@@ -45,11 +45,13 @@ class InProcessHandler {
   //! \param[in] database The path to a Crashpad database.
   //! \param[in] url The URL of an upload server.
   //! \param[in] annotations Process annotations to set in each crash report.
+  //! \param[in] system_data An object containing various system data points.
   //! \return `true` if a handler to a pending intermediate dump could be
   //!     opened.
   bool Initialize(const base::FilePath& database,
                   const std::string& url,
-                  const std::map<std::string, std::string>& annotations);
+                  const std::map<std::string, std::string>& annotations,
+                  const IOSSystemDataCollector& system_data);
 
   //! \brief Generate an intermediate dump from a signal handler exception.
   //!
@@ -192,6 +194,7 @@ class InProcessHandler {
   std::unique_ptr<IOSIntermediateDumpWriter> alternate_mach_writer_;
   std::unique_ptr<CrashReportUploadThread> upload_thread_;
   std::unique_ptr<CrashReportDatabase> database_;
+  std::string bundle_identifier_and_seperator_;
   InitializationStateDcheck initialized_;
 };
 

util/ios/ios_intermediate_dump_writer.cc

@@ -50,15 +50,6 @@ bool RawLoggingCloseFile(int fd) {
   return rv == 0;
 }
 
-// Similar to LoggingLockFile but with CRASHPAD_RAW_LOG.
-bool RawLoggingLockFileExclusiveNonBlocking(int fd) {
-  int rv = HANDLE_EINTR(flock(fd, LOCK_EX | LOCK_NB));
-  if (rv != 0) {
-    CRASHPAD_RAW_LOG_ERROR(rv, "RawLoggingLockFileExclusiveNonBlocking");
-  }
-  return rv == 0;
-}
-
 bool IOSIntermediateDumpWriter::Open(const base::FilePath& path) {
   // Set data protection class D (No protection). A file with this type of
   // protection can be read from or written to at any time.
@@ -76,7 +67,7 @@ bool IOSIntermediateDumpWriter::Open(const base::FilePath& path) {
     return false;
   }
 
-  return RawLoggingLockFileExclusiveNonBlocking(fd_);
+  return true;
 }
 
 bool IOSIntermediateDumpWriter::Close() {

util/ios/ios_intermediate_dump_writer_test.cc

@@ -54,18 +54,6 @@ class IOSIntermediateDumpWriterTest : public testing::Test {
   base::FilePath path_;
 };
 
-// Test file is locked.
-TEST_F(IOSIntermediateDumpWriterTest, OpenLocked) {
-  EXPECT_TRUE(writer_->Open(path()));
-
-  ScopedFileHandle handle(LoggingOpenFileForRead(path()));
-  EXPECT_TRUE(handle.is_valid());
-  EXPECT_EQ(LoggingLockFile(handle.get(),
-                            FileLocking::kExclusive,
-                            FileLockingBlocking::kNonBlocking),
-            FileLockingResult::kWouldBlock);
-}
-
 TEST_F(IOSIntermediateDumpWriterTest, Close) {
   EXPECT_TRUE(writer_->Open(path()));
   EXPECT_TRUE(writer_->Close());

util/ios/ios_system_data_collector.h

@@ -32,6 +32,7 @@ class IOSSystemDataCollector {
   const std::string& MachineDescription() const { return machine_description_; }
   int ProcessorCount() const { return processor_count_; }
   const std::string& Build() const { return build_; }
+  const std::string& BundleIdentifier() const { return bundle_identifier_; }
   const std::string& CPUVendor() const { return cpu_vendor_; }
   bool HasDaylightSavingTime() const { return has_next_daylight_saving_time_; }
   bool IsDaylightSavingTime() const { return is_daylight_saving_time_; }
@@ -66,6 +67,7 @@ class IOSSystemDataCollector {
   int minor_version_;
   int patch_version_;
   std::string build_;
+  std::string bundle_identifier_;
   std::string machine_description_;
   int orientation_;
   int processor_count_;

util/ios/ios_system_data_collector.mm

@@ -77,6 +77,8 @@ IOSSystemDataCollector::IOSSystemDataCollector()
   processor_count_ =
       base::saturated_cast<int>([[NSProcessInfo processInfo] processorCount]);
   build_ = ReadStringSysctlByName("kern.osversion");
+  bundle_identifier_ =
+      base::SysNSStringToUTF8([[NSBundle mainBundle] bundleIdentifier]);
 
 #if defined(ARCH_CPU_X86_64)
   cpu_vendor_ = ReadStringSysctlByName("machdep.cpu.vendor");