From a2ab062f46775b1d632c4c437daade7c530422dc Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Wed, 5 May 2021 14:15:49 -0400 Subject: [PATCH] Add non-blocking support to LoggingLockFile. Bug: crashpad: 31 Change-Id: I67689eb1cb97c1feefe51e355a1509d97d2a0735 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2871970 Commit-Queue: Justin Cohen Reviewed-by: Mark Mentovai --- client/settings.cc | 5 +++- util/file/file_io.h | 38 +++++++++++++++++++++++--- util/file/file_io_posix.cc | 17 +++++++++--- util/file/file_io_test.cc | 55 +++++++++++++++++++++++++++++++++++--- util/file/file_io_win.cc | 13 ++++++--- 5 files changed, 113 insertions(+), 15 deletions(-) diff --git a/client/settings.cc b/client/settings.cc index 0aa525f1..3855b9b6 100644 --- a/client/settings.cc +++ b/client/settings.cc @@ -207,8 +207,11 @@ Settings::ScopedLockedFileHandle Settings::MakeScopedLockedFileHandle( return ScopedLockedFileHandle(scoped.release(), base::FilePath()); #else if (scoped.is_valid()) { - if (!LoggingLockFile(scoped.get(), locking)) + if (LoggingLockFile( + scoped.get(), locking, FileLockingBlocking::kBlocking) != + FileLockingResult::kSuccess) { scoped.reset(); + } } return ScopedLockedFileHandle(scoped.release()); #endif diff --git a/util/file/file_io.h b/util/file/file_io.h index 1f502ad4..3b6d7679 100644 --- a/util/file/file_io.h +++ b/util/file/file_io.h @@ -96,6 +96,28 @@ enum class FileLocking : bool { kExclusive, }; +//! \brief Determines if LoggingLockFile will block. +enum class FileLockingBlocking : bool { + //! \brief Block until the lock is acquired + kBlocking = false, + + //! \brief Do not block when attempting to acquire a lock. + kNonBlocking = true, +}; + +//! \brief The return value for LoggingLockFile. +enum class FileLockingResult : int { + //! \brief The lock was acquired successfully. + kSuccess, + + //! \brief In non-blocking mode only, the file was already locked. Locking + //! would block, so the lock was not acquired. + kWouldBlock, + + //! \brief The lock was not acquired. + kFailure, +}; + //! \brief Determines the FileHandle that StdioFileHandle() returns. enum class StdioStream { //! \brief Standard input, or `stdin`. @@ -443,8 +465,9 @@ FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path, //! Windows. //! //! It is an error to attempt to lock a file in a different mode when it is -//! already locked. This call will block until the lock is acquired. The -//! entire file is locked. +//! already locked. This call will block until the lock is acquired unless +//! \a blocking is FileLockingBlocking::kNonBlocking. The entire file is +//! locked. //! //! If \a locking is FileLocking::kShared, \a file must have been opened for //! reading, and if it's FileLocking::kExclusive, \a file must have been opened @@ -453,9 +476,16 @@ FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path, //! \param[in] file The open file handle to be locked. //! \param[in] locking Controls whether the lock is a shared reader lock, or an //! exclusive writer lock. +//! \param[in] blocking Controls whether a locked file will result in blocking +//! or an immediate return. //! -//! \return `true` on success, or `false` and a message will be logged. -bool LoggingLockFile(FileHandle file, FileLocking locking); +//! \return kSuccess if a lock is acquired. If a lock could not be acquired +//! because \a blocking is FileLockingBlocking::kNonBlocking and acquiring +//! the lock would block, returns kWouldBlock. If a lock fails for any other +//! reason, returns kFailure and a message will be logged. +FileLockingResult LoggingLockFile(FileHandle file, + FileLocking locking, + FileLockingBlocking blocking); //! \brief Unlocks a file previously locked with LoggingLockFile(). //! diff --git a/util/file/file_io_posix.cc b/util/file/file_io_posix.cc index 2baa8114..119a2fcf 100644 --- a/util/file/file_io_posix.cc +++ b/util/file/file_io_posix.cc @@ -210,11 +210,22 @@ FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path, #if !defined(OS_FUCHSIA) -bool LoggingLockFile(FileHandle file, FileLocking locking) { +FileLockingResult LoggingLockFile(FileHandle file, + FileLocking locking, + FileLockingBlocking blocking) { int operation = (locking == FileLocking::kShared) ? LOCK_SH : LOCK_EX; + if (blocking == FileLockingBlocking::kNonBlocking) + operation |= LOCK_NB; + int rv = HANDLE_EINTR(flock(file, operation)); - PLOG_IF(ERROR, rv != 0) << "flock"; - return rv == 0; + if (rv != 0) { + if (errno == EWOULDBLOCK) { + return FileLockingResult::kWouldBlock; + } + PLOG(ERROR) << "flock"; + return FileLockingResult::kFailure; + } + return FileLockingResult::kSuccess; } bool LoggingUnlockFile(FileHandle file) { diff --git a/util/file/file_io_test.cc b/util/file/file_io_test.cc index 52bb6eee..553d9d69 100644 --- a/util/file/file_io_test.cc +++ b/util/file/file_io_test.cc @@ -561,11 +561,17 @@ TEST(FileIO, MultipleSharedLocks) { auto handle1 = ScopedFileHandle(LoggingOpenFileForRead(shared_file)); ASSERT_NE(handle1, kInvalidFileHandle); - EXPECT_TRUE(LoggingLockFile(handle1.get(), FileLocking::kShared)); + EXPECT_EQ( + LoggingLockFile( + handle1.get(), FileLocking::kShared, FileLockingBlocking::kBlocking), + FileLockingResult::kSuccess); auto handle2 = ScopedFileHandle(LoggingOpenFileForRead(shared_file)); ASSERT_NE(handle1, kInvalidFileHandle); - EXPECT_TRUE(LoggingLockFile(handle2.get(), FileLocking::kShared)); + EXPECT_EQ( + LoggingLockFile( + handle2.get(), FileLocking::kShared, FileLockingBlocking::kBlocking), + FileLockingResult::kSuccess); EXPECT_TRUE(LoggingUnlockFile(handle1.get())); EXPECT_TRUE(LoggingUnlockFile(handle2.get())); @@ -590,7 +596,9 @@ class LockingTestThread : public Thread { private: void ThreadMain() override { for (int i = 0; i < iterations_; ++i) { - EXPECT_TRUE(LoggingLockFile(file_.get(), lock_type_)); + EXPECT_EQ(LoggingLockFile( + file_.get(), lock_type_, FileLockingBlocking::kBlocking), + FileLockingResult::kSuccess); base::subtle::NoBarrier_AtomicIncrement(actual_iterations_, 1); EXPECT_TRUE(LoggingUnlockFile(file_.get())); } @@ -624,7 +632,9 @@ void LockingTest(FileLocking main_lock, FileLocking other_locks) { FileWriteMode::kReuseOrCreate, FilePermissions::kOwnerOnly)); ASSERT_NE(initial, kInvalidFileHandle); - ASSERT_TRUE(LoggingLockFile(initial.get(), main_lock)); + EXPECT_EQ( + LoggingLockFile(initial.get(), main_lock, FileLockingBlocking::kBlocking), + FileLockingResult::kSuccess); base::subtle::Atomic32 actual_iterations = 0; @@ -671,6 +681,43 @@ TEST(FileIO, SharedVsExclusives) { LockingTest(FileLocking::kShared, FileLocking::kExclusive); } +TEST(FileIO, ExclusiveVsExclusivesNonBlocking) { + ScopedTempDir temp_dir; + base::FilePath exclusive_file = + temp_dir.path().Append(FILE_PATH_LITERAL("file_to_lock")); + + { + // Create an empty file to lock. + ScopedFileHandle create( + LoggingOpenFileForWrite(exclusive_file, + FileWriteMode::kCreateOrFail, + FilePermissions::kOwnerOnly)); + } + + auto handle1 = ScopedFileHandle(LoggingOpenFileForRead(exclusive_file)); + ASSERT_NE(handle1, kInvalidFileHandle); + EXPECT_EQ(LoggingLockFile(handle1.get(), + FileLocking::kExclusive, + FileLockingBlocking::kBlocking), + FileLockingResult::kSuccess); + + // Non-blocking lock should fail. + auto handle2 = ScopedFileHandle(LoggingOpenFileForRead(exclusive_file)); + ASSERT_NE(handle2, kInvalidFileHandle); + EXPECT_EQ(LoggingLockFile(handle2.get(), + FileLocking::kExclusive, + FileLockingBlocking::kNonBlocking), + FileLockingResult::kWouldBlock); + + // After unlocking, non-blocking lock should succeed. + EXPECT_TRUE(LoggingUnlockFile(handle1.get())); + EXPECT_EQ(LoggingLockFile(handle2.get(), + FileLocking::kExclusive, + FileLockingBlocking::kNonBlocking), + FileLockingResult::kSuccess); + EXPECT_TRUE(LoggingUnlockFile(handle2.get())); +} + #endif // !OS_FUCHSIA TEST(FileIO, FileSizeByHandle) { diff --git a/util/file/file_io_win.cc b/util/file/file_io_win.cc index 8586619d..93a12294 100644 --- a/util/file/file_io_win.cc +++ b/util/file/file_io_win.cc @@ -182,19 +182,26 @@ FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path, return file; } -bool LoggingLockFile(FileHandle file, FileLocking locking) { +FileLockingResult LoggingLockFile(FileHandle file, + FileLocking locking, + FileLockingBlocking blocking) { DWORD flags = (locking == FileLocking::kExclusive) ? LOCKFILE_EXCLUSIVE_LOCK : 0; + if (blocking == FileLockingBlocking::kNonBlocking) + flags |= LOCKFILE_FAIL_IMMEDIATELY; // Note that the `Offset` fields of overlapped indicate the start location for // locking (beginning of file in this case), and `hEvent` must be also be set // to 0. OVERLAPPED overlapped = {0}; if (!LockFileEx(file, flags, 0, MAXDWORD, MAXDWORD, &overlapped)) { + if (GetLastError() == ERROR_LOCK_VIOLATION) { + return FileLockingResult::kWouldBlock; + } PLOG(ERROR) << "LockFileEx"; - return false; + return FileLockingResult::kFailure; } - return true; + return FileLockingResult::kSuccess; } bool LoggingUnlockFile(FileHandle file) {