fuchsia: Compile out LoggingLock/UnlockFile, add DCHECKs to Settings

Fuchsia does not currently support any sort of file locking. Until a
lock server can be implemented, compile out the calls to flock(). In the
one current non-test user of locking (Settings) add a
pseudo-implementation that will DCHECK if there is ever contention on
the lock.

Bug: crashpad:217, crashpad:196
Change-Id: Ifdf7e00886ad7e7778745f1ae8f0ce2a86f0ae3b
Reviewed-on: https://chromium-review.googlesource.com/924312
Commit-Queue: Scott Graham <scottmg@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>

client/settings.cc

@@ -20,10 +20,55 @@
 
 #include "base/logging.h"
 #include "base/posix/eintr_wrapper.h"
+#include "util/file/filesystem.h"
 #include "util/numeric/in_range_cast.h"
 
 namespace crashpad {
 
+#if defined(OS_FUCHSIA)
+
+Settings::ScopedLockedFileHandle::ScopedLockedFileHandle()
+    : handle_(kInvalidFileHandle), lockfile_path_() {
+    }
+
+Settings::ScopedLockedFileHandle::ScopedLockedFileHandle(
+    FileHandle handle,
+    const base::FilePath& lockfile_path)
+    : handle_(handle), lockfile_path_(lockfile_path) {
+}
+
+Settings::ScopedLockedFileHandle::ScopedLockedFileHandle(
+    ScopedLockedFileHandle&& other)
+    : handle_(other.handle_), lockfile_path_(other.lockfile_path_) {
+  other.handle_ = kInvalidFileHandle;
+  other.lockfile_path_ = base::FilePath();
+}
+
+Settings::ScopedLockedFileHandle& Settings::ScopedLockedFileHandle::operator=(
+    ScopedLockedFileHandle&& other) {
+  handle_ = other.handle_;
+  lockfile_path_ = other.lockfile_path_;
+
+  other.handle_ = kInvalidFileHandle;
+  other.lockfile_path_ = base::FilePath();
+  return *this;
+}
+
+Settings::ScopedLockedFileHandle::~ScopedLockedFileHandle() {
+  Destroy();
+}
+
+void Settings::ScopedLockedFileHandle::Destroy() {
+  if (handle_ != kInvalidFileHandle) {
+    CheckedCloseFile(handle_);
+  }
+  if (!lockfile_path_.empty()) {
+    DCHECK(LoggingRemoveFile(lockfile_path_));
+  }
+}
+
+#else // OS_FUCHSIA
+
 namespace internal {
 
 // static
@@ -36,6 +81,8 @@ void ScopedLockedFileHandleTraits::Free(FileHandle handle) {
 
 }  // namespace internal
 
+#endif  // OS_FUCHSIA
+
 struct Settings::Data {
   static const uint32_t kSettingsMagic = 'CPds';
   static const uint32_t kSettingsVersion = 1;
@@ -142,18 +189,33 @@ bool Settings::SetLastUploadAttemptTime(time_t time) {
 // static
 Settings::ScopedLockedFileHandle Settings::MakeScopedLockedFileHandle(
     FileHandle file,
-    FileLocking locking) {
+    FileLocking locking,
+    const base::FilePath& file_path) {
   ScopedFileHandle scoped(file);
+#if defined(OS_FUCHSIA)
+  base::FilePath lockfile_path(file_path.value() + ".__lock__");
+  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());
+#else
   if (scoped.is_valid()) {
     if (!LoggingLockFile(scoped.get(), locking))
       scoped.reset();
   }
   return ScopedLockedFileHandle(scoped.release());
+#endif
 }
 
 Settings::ScopedLockedFileHandle Settings::OpenForReading() {
-  return MakeScopedLockedFileHandle(LoggingOpenFileForRead(file_path()),
+  return MakeScopedLockedFileHandle(
-                                    FileLocking::kShared);
+      LoggingOpenFileForRead(file_path()), FileLocking::kShared, file_path());
 }
 
 Settings::ScopedLockedFileHandle Settings::OpenForReadingAndWriting(
@@ -169,7 +231,8 @@ Settings::ScopedLockedFileHandle Settings::OpenForReadingAndWriting(
         file_path(), mode, FilePermissions::kWorldReadable);
   }
 
-  return MakeScopedLockedFileHandle(handle, FileLocking::kExclusive);
+  return MakeScopedLockedFileHandle(
+      handle, FileLocking::kExclusive, file_path());
 }
 
 bool Settings::OpenAndReadSettings(Data* out_data) {

client/settings.h

@@ -22,6 +22,7 @@
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/scoped_generic.h"
+#include "build/build_config.h"
 #include "util/file/file_io.h"
 #include "util/misc/initialization_state.h"
 #include "util/misc/uuid.h"
@@ -114,11 +115,45 @@ class Settings {
   struct Data;
 
   // This must be constructed with MakeScopedLockedFileHandle(). It both unlocks
-  // and closes the file on destruction.
+  // 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 defined(OS_FUCHSIA)
+  struct ScopedLockedFileHandle {
+   public:
+    ScopedLockedFileHandle();
+    ScopedLockedFileHandle(FileHandle handle,
+                           const base::FilePath& lockfile_path);
+    ScopedLockedFileHandle(ScopedLockedFileHandle&& other);
+    ScopedLockedFileHandle& operator=(ScopedLockedFileHandle&& other);
+    ~ScopedLockedFileHandle();
+
+    // These mirror the non-Fuchsia ScopedLockedFileHandle via ScopedGeneric so
+    // that calling code can pretend this implementation is the same.
+    bool is_valid() const { return handle_ != kInvalidFileHandle; }
+    FileHandle get() { return handle_; }
+    void reset() {
+      Destroy();
+      handle_ = kInvalidFileHandle;
+      lockfile_path_ = base::FilePath();
+    }
+
+   private:
+    void Destroy();
+
+    FileHandle handle_;
+    base::FilePath lockfile_path_;
+
+    DISALLOW_COPY_AND_ASSIGN(ScopedLockedFileHandle);
+  };
+#else  // OS_FUCHSIA
   using ScopedLockedFileHandle =
       base::ScopedGeneric<FileHandle, internal::ScopedLockedFileHandleTraits>;
-  static ScopedLockedFileHandle MakeScopedLockedFileHandle(FileHandle file,
+#endif  // OS_FUCHSIA
-                                                           FileLocking locking);
+  static ScopedLockedFileHandle MakeScopedLockedFileHandle(
+      FileHandle file,
+      FileLocking locking,
+      const base::FilePath& file_path);
 
   // Opens the settings file for reading. On error, logs a message and returns
   // the invalid handle.

util/file/file_io.h

@@ -404,6 +404,11 @@ FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path,
                                           FileWriteMode mode,
                                           FilePermissions permissions);
 
+// 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 !defined(OS_FUCHSIA)
+
 //! \brief Locks the given \a file using `flock()` on POSIX or `LockFileEx()` on
 //!     Windows.
 //!
@@ -433,6 +438,8 @@ bool LoggingLockFile(FileHandle file, FileLocking locking);
 //! \return `true` on success, or `false` and a message will be logged.
 bool LoggingUnlockFile(FileHandle file);
 
+#endif  // !OS_FUCHSIA
+
 //! \brief Wraps `lseek()` or `SetFilePointerEx()`. Logs an error if the
 //!     operation fails.
 //!

util/file/file_io_posix.cc

@@ -157,18 +157,7 @@ FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path,
   return fd;
 }
 
-#if defined(OS_FUCHSIA)
+#if !defined(OS_FUCHSIA)
-int flock(int, int) {
-  // TODO(scottmg): https://crashpad.chromium.org/bug/196:
-  // This was removed from the libc of Fuchsia recently. A new implementation of
-  // Settings is being worked on that doesn't require flock(), but until then,
-  // it's more useful to have it link, but fail at runtime than it is to exclude
-  // a lot of code (Settings, which requires excludes the CrashReportDatabase,
-  // which requires excluding a variety of tests, the handler, and so on.).
-  NOTREACHED();
-  return ENOSYS;
-}
-#endif  // OS_FUCHSIA
 
 bool LoggingLockFile(FileHandle file, FileLocking locking) {
   int operation = (locking == FileLocking::kShared) ? LOCK_SH : LOCK_EX;
@@ -183,6 +172,8 @@ bool LoggingUnlockFile(FileHandle file) {
   return rv == 0;
 }
 
+#endif  // !OS_FUCHSIA
+
 FileOffset LoggingSeekFile(FileHandle file, FileOffset offset, int whence) {
   off_t rv = lseek(file, offset, whence);
   PLOG_IF(ERROR, rv < 0) << "lseek";

util/file/file_io_test.cc

@@ -523,6 +523,11 @@ TEST(FileIO, FileShareMode_Write_Write) {
   FileShareModeTest(ReadOrWrite::kWrite, ReadOrWrite::kWrite);
 }
 
+// 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 !defined(OS_FUCHSIA)
+
 TEST(FileIO, MultipleSharedLocks) {
   ScopedTempDir temp_dir;
   base::FilePath shared_file =
@@ -648,6 +653,8 @@ TEST(FileIO, SharedVsExclusives) {
   LockingTest(FileLocking::kShared, FileLocking::kExclusive);
 }
 
+#endif  // !OS_FUCHSIA
+
 TEST(FileIO, FileSizeByHandle) {
   EXPECT_EQ(LoggingFileSizeByHandle(kInvalidFileHandle), -1);