Fix locking on certain Android partitions

Copy of crrev.com/c/3952963.

Fixes locking not working on some Android filesystems due to flock not
being available. Instead, we now use the same approach as Fuchsia with
a dedicated lock file. This is an issue when running tests on
non-rooted Android devices, as we need files to be written to a
location accessible without root, but the chosen location might not
have flock support.

Bug: chromium:1358240
Change-Id: Ie910481be472403a8b0e9e36100594b0618f85e6
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3999273
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: Joshua Peraza <jperaza@chromium.org>

build/BUILD.gn

@@ -30,6 +30,12 @@ config("crashpad_is_in_fuchsia") {
   }
 }
 
+config("flock_always_supported_defines") {
+  defines = [
+    "CRASHPAD_FLOCK_ALWAYS_SUPPORTED=$crashpad_flock_always_supported",
+  ]
+}
+
 group("default_exe_manifest_win") {
   if (crashpad_is_in_chromium) {
     deps = [ "//build/win:default_exe_manifest" ]

build/crashpad_buildconfig.gni

@@ -81,6 +81,8 @@ if (crashpad_is_in_chromium) {
   crashpad_is_clang = mini_chromium_is_clang
 }
 
+crashpad_flock_always_supported = !(crashpad_is_android || crashpad_is_fuchsia)
+
 template("crashpad_executable") {
   executable(target_name) {
     forward_variables_from(invoker,

client/BUILD.gn

@@ -144,6 +144,7 @@ static_library("common") {
     "../util",
   ]
   deps = [ "../util" ]
+  configs += [ "../build:flock_always_supported_defines" ]
 }
 
 source_set("client_test") {

client/crash_report_database_generic.cc

@@ -549,6 +549,16 @@ int CrashReportDatabaseGeneric::CleanDatabase(time_t lockfile_ttl) {
   removed += CleanReportsInState(kPending, lockfile_ttl);
   removed += CleanReportsInState(kCompleted, lockfile_ttl);
   CleanOrphanedAttachments();
+#if !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
+  base::FilePath settings_path(kSettings);
+  if (Settings::IsLockExpired(settings_path, lockfile_ttl)) {
+    base::FilePath lockfile_path(settings_path.value() +
+                                 Settings::kLockfileExtension);
+    if (LoggingRemoveFile(lockfile_path)) {
+      ++removed;
+    }
+  }
+#endif  // !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
   return removed;
 }
 

client/settings.cc

@@ -26,7 +26,7 @@
 
 namespace crashpad {
 
-#if BUILDFLAG(IS_FUCHSIA)
+#if !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
 
 Settings::ScopedLockedFileHandle::ScopedLockedFileHandle()
     : handle_(kInvalidFileHandle), lockfile_path_() {
@@ -69,7 +69,7 @@ void Settings::ScopedLockedFileHandle::Destroy() {
   }
 }
 
-#else  // BUILDFLAG(IS_FUCHSIA)
+#else  // !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
 
 #if BUILDFLAG(IS_IOS)
 
@@ -217,25 +217,50 @@ bool Settings::SetLastUploadAttemptTime(time_t time) {
   return WriteSettings(handle.get(), settings);
 }
 
+#if !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
+// static
+bool Settings::IsLockExpired(const base::FilePath& file_path,
+                             time_t lockfile_ttl) {
+  time_t now = time(nullptr);
+  base::FilePath lock_path(file_path.value() + Settings::kLockfileExtension);
+  ScopedFileHandle lock_fd(LoggingOpenFileForRead(lock_path));
+  time_t lock_timestamp;
+  if (!LoggingReadFileExactly(
+          lock_fd.get(), &lock_timestamp, sizeof(lock_timestamp))) {
+    return false;
+  }
+  return now >= lock_timestamp + lockfile_ttl;
+}
+#endif  // !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
+
 // static
 Settings::ScopedLockedFileHandle Settings::MakeScopedLockedFileHandle(
-    FileHandle file,
+    const internal::MakeScopedLockedFileHandleOptions& options,
     FileLocking locking,
     const base::FilePath& file_path) {
-  ScopedFileHandle scoped(file);
-#if BUILDFLAG(IS_FUCHSIA)
-  base::FilePath lockfile_path(file_path.value() + ".__lock__");
+#if !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
+  base::FilePath lockfile_path(file_path.value() +
+                               Settings::kLockfileExtension);
+  ScopedFileHandle lockfile_scoped(
+      LoggingOpenFileForWrite(lockfile_path,
+                              FileWriteMode::kCreateOrFail,
+                              FilePermissions::kWorldReadable));
+  if (!lockfile_scoped.is_valid()) {
+    return ScopedLockedFileHandle();
+  }
+  time_t now = time(nullptr);
+  if (!LoggingWriteFile(lockfile_scoped.get(), &now, sizeof(now))) {
+    return ScopedLockedFileHandle();
+  }
+  ScopedFileHandle scoped(GetHandleFromOptions(file_path, options));
   if (scoped.is_valid()) {
-    ScopedFileHandle lockfile_scoped(
-        LoggingOpenFileForWrite(lockfile_path,
-                                FileWriteMode::kCreateOrFail,
-                                FilePermissions::kWorldReadable));
-    // This is a lightweight attempt to try to catch racy behavior.
-    DCHECK(lockfile_scoped.is_valid());
     return ScopedLockedFileHandle(scoped.release(), lockfile_path);
   }
-  return ScopedLockedFileHandle(scoped.release(), base::FilePath());
+  bool success = LoggingRemoveFile(lockfile_path);
+  DCHECK(success);
+  return ScopedLockedFileHandle();
 #else
+  ScopedFileHandle scoped(GetHandleFromOptions(file_path, options));
   // It's important to create the ScopedLockedFileHandle before calling
   // LoggingLockFile on iOS, so a ScopedBackgroundTask is created *before*
   // the LoggingLockFile call below.
@@ -249,29 +274,50 @@ Settings::ScopedLockedFileHandle Settings::MakeScopedLockedFileHandle(
   }
   handle.reset(scoped.release());
   return handle;
-#endif
+#endif  // !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
+}
+
+// static
+FileHandle Settings::GetHandleFromOptions(
+    const base::FilePath& file_path,
+    const internal::MakeScopedLockedFileHandleOptions& options) {
+  switch (options.function_enum) {
+    case internal::FileOpenFunction::kLoggingOpenFileForRead:
+      return LoggingOpenFileForRead(file_path);
+    case internal::FileOpenFunction::kLoggingOpenFileForReadAndWrite:
+      return LoggingOpenFileForReadAndWrite(
+          file_path, options.mode, options.permissions);
+    case internal::FileOpenFunction::kOpenFileForReadAndWrite:
+      return OpenFileForReadAndWrite(
+          file_path, options.mode, options.permissions);
+  }
+  NOTREACHED();
+  return kInvalidFileHandle;
 }
 
 Settings::ScopedLockedFileHandle Settings::OpenForReading() {
-  return MakeScopedLockedFileHandle(
-      LoggingOpenFileForRead(file_path()), FileLocking::kShared, file_path());
+  internal::MakeScopedLockedFileHandleOptions options;
+  options.function_enum = internal::FileOpenFunction::kLoggingOpenFileForRead;
+  return MakeScopedLockedFileHandle(options, FileLocking::kShared, file_path());
 }
 
 Settings::ScopedLockedFileHandle Settings::OpenForReadingAndWriting(
     FileWriteMode mode, bool log_open_error) {
   DCHECK(mode != FileWriteMode::kTruncateOrCreate);
 
-  FileHandle handle;
+  internal::MakeScopedLockedFileHandleOptions options;
+  options.mode = mode;
+  options.permissions = FilePermissions::kOwnerOnly;
   if (log_open_error) {
-    handle = LoggingOpenFileForReadAndWrite(
-        file_path(), mode, FilePermissions::kOwnerOnly);
+    options.function_enum =
+        internal::FileOpenFunction::kLoggingOpenFileForReadAndWrite;
   } else {
-    handle = OpenFileForReadAndWrite(
-        file_path(), mode, FilePermissions::kOwnerOnly);
+    options.function_enum =
+        internal::FileOpenFunction::kOpenFileForReadAndWrite;
   }
 
   return MakeScopedLockedFileHandle(
-      handle, FileLocking::kExclusive, file_path());
+      options, FileLocking::kExclusive, file_path());
 }
 
 bool Settings::OpenAndReadSettings(Data* out_data) {

client/settings.h

@@ -37,6 +37,18 @@ struct ScopedLockedFileHandleTraits {
   static void Free(FileHandle handle);
 };
 
+enum class FileOpenFunction {
+  kLoggingOpenFileForRead,
+  kLoggingOpenFileForReadAndWrite,
+  kOpenFileForReadAndWrite,
+};
+
+struct MakeScopedLockedFileHandleOptions {
+  FileOpenFunction function_enum;
+  FileWriteMode mode;
+  FilePermissions permissions;
+};
+
 // TODO(mark): The timeout should be configurable by the client.
 #if BUILDFLAG(IS_IOS)
 // iOS background assertions only last 30 seconds, keep the timeout shorter.
@@ -54,6 +66,8 @@ constexpr double kUploadReportTimeoutSeconds = 60;
 //! should be retrieved via CrashReportDatabase::GetSettings().
 class Settings {
  public:
+  static inline constexpr char kLockfileExtension[] = ".__lock__";
+
   Settings();
 
   Settings(const Settings&) = delete;
@@ -128,6 +142,20 @@ class Settings {
   //!     error logged.
   bool SetLastUploadAttemptTime(time_t time);
 
+#if !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
+  //! \brief Returns whether the lockfile for a file is expired.
+  //!
+  //! This could be part of ScopedLockedFileHandle, but this needs to be
+  //! public while ScopedLockedFileHandle is private to Settings.
+  //!
+  //! \param[in] file_path The path to the file whose lockfile will be checked.
+  //! \param[in] lockfile_ttl How long the lockfile has to live before expiring.
+  //!
+  //! \return `true` if the lock for the file is expired, otherwise `false`.
+  static bool IsLockExpired(const base::FilePath& file_path,
+                            time_t lockfile_ttl);
+#endif  // !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
+
  private:
   struct Data;
 
@@ -135,7 +163,7 @@ class Settings {
   // and closes the file on destruction. Note that on Fuchsia, this handle DOES
   // NOT offer correct operation, only an attempt to DCHECK if racy behavior is
   // detected.
-#if BUILDFLAG(IS_FUCHSIA)
+#if !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
   struct ScopedLockedFileHandle {
    public:
     ScopedLockedFileHandle();
@@ -187,12 +215,16 @@ class Settings {
 #else
   using ScopedLockedFileHandle =
       base::ScopedGeneric<FileHandle, internal::ScopedLockedFileHandleTraits>;
-#endif  // BUILDFLAG(IS_FUCHSIA)
+#endif  // !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
   static ScopedLockedFileHandle MakeScopedLockedFileHandle(
-      FileHandle file,
+      const internal::MakeScopedLockedFileHandleOptions& options,
       FileLocking locking,
       const base::FilePath& file_path);
 
+  static FileHandle GetHandleFromOptions(
+      const base::FilePath& file_path,
+      const internal::MakeScopedLockedFileHandleOptions& options);
+
   // Opens the settings file for reading. On error, logs a message and returns
   // the invalid handle.
   ScopedLockedFileHandle OpenForReading();

util/BUILD.gn

@@ -577,6 +577,8 @@ crashpad_static_library("util") {
     "$mini_chromium_source_parent:chromeos_buildflags",
   ]
 
+  configs = [ "../build:flock_always_supported_defines" ]
+
   if (crashpad_is_mac || crashpad_is_ios) {
     include_dirs += [ "$root_gen_dir" ]
     deps += [ ":mig_output" ]

util/file/file_io.h

@@ -461,7 +461,7 @@ FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path,
 // Fuchsia does not currently support any sort of file locking. See
 // https://crashpad.chromium.org/bug/196 and
 // https://crashpad.chromium.org/bug/217.
-#if !BUILDFLAG(IS_FUCHSIA)
+#if CRASHPAD_FLOCK_ALWAYS_SUPPORTED
 
 //! \brief Locks the given \a file using `flock()` on POSIX or `LockFileEx()` on
 //!     Windows.
@@ -500,7 +500,7 @@ FileLockingResult LoggingLockFile(FileHandle file,
 //! \return `true` on success, or `false` and a message will be logged.
 bool LoggingUnlockFile(FileHandle file);
 
-#endif  // !BUILDFLAG(IS_FUCHSIA)
+#endif  // CRASHPAD_FLOCK_ALWAYS_SUPPORTED
 
 //! \brief Wraps `lseek()` or `SetFilePointerEx()`. Logs an error if the
 //!     operation fails.

util/file/file_io_posix.cc

@@ -208,7 +208,7 @@ FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path,
   return fd;
 }
 
-#if !BUILDFLAG(IS_FUCHSIA)
+#if CRASHPAD_FLOCK_ALWAYS_SUPPORTED
 
 FileLockingResult LoggingLockFile(FileHandle file,
                                   FileLocking locking,
@@ -234,7 +234,7 @@ bool LoggingUnlockFile(FileHandle file) {
   return rv == 0;
 }
 
-#endif  // !BUILDFLAG(IS_FUCHSIA)
+#endif  // CRASHPAD_FLOCK_ALWAYS_SUPPORTED
 
 FileOffset LoggingSeekFile(FileHandle file, FileOffset offset, int whence) {
   off_t rv = lseek(file, offset, whence);

util/file/file_io_test.cc

@@ -546,7 +546,9 @@ TEST(FileIO, FileShareMode_Write_Write) {
 // Fuchsia does not currently support any sort of file locking. See
 // https://crashpad.chromium.org/bug/196 and
 // https://crashpad.chromium.org/bug/217.
-#if !BUILDFLAG(IS_FUCHSIA)
+// Android can conditionally not support file locking depending on what type of
+// filesystem is being used to store settings.dat.
+#if CRASHPAD_FLOCK_ALWAYS_SUPPORTED
 
 TEST(FileIO, MultipleSharedLocks) {
   ScopedTempDir temp_dir;
@@ -721,7 +723,7 @@ TEST(FileIO, ExclusiveVsExclusivesNonBlocking) {
   EXPECT_TRUE(LoggingUnlockFile(handle2.get()));
 }
 
-#endif  // !BUILDFLAG(IS_FUCHSIA)
+#endif  // CRASHPAD_FLOCK_ALWAYS_SUPPORTED
 
 TEST(FileIO, FileSizeByHandle) {
   EXPECT_EQ(LoggingFileSizeByHandle(kInvalidFileHandle), -1);