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: Ifbacf2905136669195ee631460ae004d44c959ca
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>
GitOrigin-RevId: c367128a856a7eb161f0db6a5a1dc5c8d9d16cd3
diff --git a/client/crashpad_client_ios.cc b/client/crashpad_client_ios.cc
index 9a911df..8c00d5d 100644
--- a/client/crashpad_client_ios.cc
+++ b/client/crashpad_client_ios.cc
@@ -68,7 +68,8 @@
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.";
diff --git a/client/ios_handler/in_process_handler.cc b/client/ios_handler/in_process_handler.cc
index 85fd422..d4c4af4 100644
--- a/client/ios_handler/in_process_handler.cc
+++ b/client/ios_handler/in_process_handler.cc
@@ -36,6 +36,13 @@
}
}
+// 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 @@
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 @@
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 @@
}
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: "
diff --git a/client/ios_handler/in_process_handler.h b/client/ios_handler/in_process_handler.h
index d825c29..a9a3094 100644
--- a/client/ios_handler/in_process_handler.h
+++ b/client/ios_handler/in_process_handler.h
@@ -45,11 +45,13 @@
//! \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 @@
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_;
};
diff --git a/util/ios/ios_intermediate_dump_writer.cc b/util/ios/ios_intermediate_dump_writer.cc
index eca33d3..cd4d99d 100644
--- a/util/ios/ios_intermediate_dump_writer.cc
+++ b/util/ios/ios_intermediate_dump_writer.cc
@@ -50,15 +50,6 @@
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 @@
return false;
}
- return RawLoggingLockFileExclusiveNonBlocking(fd_);
+ return true;
}
bool IOSIntermediateDumpWriter::Close() {
diff --git a/util/ios/ios_intermediate_dump_writer_test.cc b/util/ios/ios_intermediate_dump_writer_test.cc
index 6c6bcad..24f56ef 100644
--- a/util/ios/ios_intermediate_dump_writer_test.cc
+++ b/util/ios/ios_intermediate_dump_writer_test.cc
@@ -54,18 +54,6 @@
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());
diff --git a/util/ios/ios_system_data_collector.h b/util/ios/ios_system_data_collector.h
index cac0c86..ab25a68 100644
--- a/util/ios/ios_system_data_collector.h
+++ b/util/ios/ios_system_data_collector.h
@@ -32,6 +32,7 @@
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 @@
int minor_version_;
int patch_version_;
std::string build_;
+ std::string bundle_identifier_;
std::string machine_description_;
int orientation_;
int processor_count_;
diff --git a/util/ios/ios_system_data_collector.mm b/util/ios/ios_system_data_collector.mm
index 4387ac9..ed81849 100644
--- a/util/ios/ios_system_data_collector.mm
+++ b/util/ios/ios_system_data_collector.mm
@@ -77,6 +77,8 @@
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");
