From 5cb869392eeda631f4c07c5ca1a141b5c8572b82 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Fri, 16 Feb 2018 12:24:09 -0800 Subject: [PATCH] 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 Reviewed-by: Mark Mentovai --- client/settings.cc | 71 +++++++++++++++++++++++++++++++++++--- client/settings.h | 41 ++++++++++++++++++++-- util/file/file_io.h | 7 ++++ util/file/file_io_posix.cc | 15 ++------ util/file/file_io_test.cc | 7 ++++ 5 files changed, 122 insertions(+), 19 deletions(-) diff --git a/client/settings.cc b/client/settings.cc index 8d4dbec8..20bd2581 100644 --- a/client/settings.cc +++ b/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()), - FileLocking::kShared); + return MakeScopedLockedFileHandle( + 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) { diff --git a/client/settings.h b/client/settings.h index 488080c1..a2b0c746 100644 --- a/client/settings.h +++ b/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; - static ScopedLockedFileHandle MakeScopedLockedFileHandle(FileHandle file, - FileLocking locking); +#endif // OS_FUCHSIA + 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. diff --git a/util/file/file_io.h b/util/file/file_io.h index 044a0a69..050c0749 100644 --- a/util/file/file_io.h +++ b/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. //! diff --git a/util/file/file_io_posix.cc b/util/file/file_io_posix.cc index bb6e87ea..f3116169 100644 --- a/util/file/file_io_posix.cc +++ b/util/file/file_io_posix.cc @@ -157,18 +157,7 @@ FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path, return fd; } -#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 +#if !defined(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"; diff --git a/util/file/file_io_test.cc b/util/file/file_io_test.cc index fdcf7e9a..4446c320 100644 --- a/util/file/file_io_test.cc +++ b/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);