Add Locking calls to file_io.h plus implementations and test

Towards removing use of open() with O_EXLOCK/O_SHLOCK in code
used on non-BSD.

Adds simple Thread abstraction to util/test.

Includes mini_chromium roll with:
56dd2883170d0df0ec89af0e7862af3f9aaa9be6 Fix import of atomicops for Windows
886592fd6677615c54c4156bb2f2edb5d547ba6c Export SystemErrorCodeToString

R=mark@chromium.org, rsesek@chromium.org
BUG=crashpad:1, crashpad:13

Review URL: https://codereview.chromium.org/1001673002

DEPS

@@ -28,7 +28,7 @@ deps = {
       '32ca1cd8e010d013a606a752fb49a603a3598071',  # svn r2015
   'crashpad/third_party/mini_chromium/mini_chromium':
       Var('chromium_git') + '/chromium/mini_chromium@' +
-      '31e989ac0b03d50ebe71bfa13417c7f8b66db1b8',
+      '56dd2883170d0df0ec89af0e7862af3f9aaa9be6',
 }
 
 hooks = [

util/file/file_io.h

@@ -78,6 +78,15 @@ enum class FilePermissions : bool {
   kWorldReadable,
 };
 
+//! \brief Determines the locking mode that LoggingLockFile() uses.
+enum class FileLocking : bool {
+  //! \brief Equivalent to `flock()` with `LOCK_SH`.
+  kShared,
+
+  //! \brief Equivalent to `flock()` with `LOCK_EX`.
+  kExclusive,
+};
+
 //! \brief Reads from a file, retrying when interrupted on POSIX or following a
 //!     short read.
 //!
@@ -182,7 +191,8 @@ FileHandle LoggingOpenFileForRead(const base::FilePath& path);
 //! \a write_mode determines the style (truncate, reuse, etc.) that is used to
 //! open the file. On POSIX, \a permissions determines the value that is passed
 //! as `mode` to `open()`. On Windows, the file is always opened in binary mode
-//! (that is, no CRLF translation).
+//! (that is, no CRLF translation). On Windows, the file is opened for sharing,
+//! see LoggingLockFile() and LoggingUnlockFile() to control concurrent access.
 //!
 //! \return The newly opened FileHandle, or an invalid FileHandle on failure.
 //!
@@ -193,6 +203,35 @@ FileHandle LoggingOpenFileForWrite(const base::FilePath& path,
                                    FileWriteMode write_mode,
                                    FilePermissions permissions);
 
+//! \brief Locks the given \a file using `flock()` on POSIX or `LockFileEx()` on
+//!     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.
+//!
+//! 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
+//! for writing.
+//!
+//! \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.
+//!
+//! \return `true` on success, or `false` and a message will be logged.
+bool LoggingLockFile(FileHandle file, FileLocking locking);
+
+//! \brief Unlocks a file previously locked with LoggingLockFile().
+//!
+//! It is an error to attempt to unlock a file that was not previously locked.
+//! A previously-locked file should be unlocked before closing the file handle,
+//! otherwise on some OSs the lock may not be released immediately.
+//!
+//! \param[in] file The open locked file handle to be unlocked.
+//!
+//! \return `true` on success, or `false` and a message will be logged.
+bool LoggingUnlockFile(FileHandle file);
+
 //! \brief Wraps `lseek()` or `SetFilePointerEx()`. Logs an error if the
 //!     operation fails.
 //!

util/file/file_io_posix.cc

@@ -15,6 +15,7 @@
 #include "util/file/file_io.h"
 
 #include <fcntl.h>
+#include <sys/file.h>
 #include <unistd.h>
 
 #include "base/files/file_path.h"
@@ -100,6 +101,19 @@ FileHandle LoggingOpenFileForWrite(const base::FilePath& path,
   return fd;
 }
 
+bool LoggingLockFile(FileHandle file, FileLocking locking) {
+  int operation = locking == FileLocking::kShared ? LOCK_SH : LOCK_EX;
+  int rv = HANDLE_EINTR(flock(file, operation));
+  PLOG_IF(ERROR, rv != 0) << "flock";
+  return rv == 0;
+}
+
+bool LoggingUnlockFile(FileHandle file) {
+  int rv = flock(file, LOCK_UN);
+  PLOG_IF(ERROR, rv != 0) << "flock";
+  return rv == 0;
+}
+
 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

@@ -0,0 +1,207 @@
+// Copyright 2015 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "util/file/file_io.h"
+
+#include "base/atomicops.h"
+#include "base/basictypes.h"
+#include "base/files/file_path.h"
+#include "gtest/gtest.h"
+#include "util/test/errors.h"
+#include "util/test/scoped_temp_dir.h"
+#include "util/test/thread.h"
+
+namespace crashpad {
+namespace test {
+namespace {
+
+enum class ReadOrWrite : bool {
+  kRead,
+  kWrite,
+};
+
+void FileShareModeTest(ReadOrWrite first, ReadOrWrite second) {
+  ScopedTempDir temp_dir;
+  base::FilePath shared_file =
+      temp_dir.path().Append(FILE_PATH_LITERAL("shared_file"));
+  {
+    // Create an empty file to work on.
+    ScopedFileHandle create(
+        LoggingOpenFileForWrite(shared_file,
+                                FileWriteMode::kCreateOrFail,
+                                FilePermissions::kOwnerOnly));
+  }
+
+  auto handle1 = ScopedFileHandle(
+      (first == ReadOrWrite::kRead)
+          ? LoggingOpenFileForRead(shared_file)
+          : LoggingOpenFileForWrite(shared_file,
+                                    FileWriteMode::kReuseOrCreate,
+                                    FilePermissions::kOwnerOnly));
+  ASSERT_NE(handle1, kInvalidFileHandle);
+  auto handle2 = ScopedFileHandle(
+      (second == ReadOrWrite::kRead)
+          ? LoggingOpenFileForRead(shared_file)
+          : LoggingOpenFileForWrite(shared_file,
+                                    FileWriteMode::kReuseOrCreate,
+                                    FilePermissions::kOwnerOnly));
+  EXPECT_NE(handle2, kInvalidFileHandle);
+
+  EXPECT_NE(handle1.get(), handle2.get());
+}
+
+TEST(FileIO, FileShareMode_Read_Read) {
+  FileShareModeTest(ReadOrWrite::kRead, ReadOrWrite::kRead);
+}
+
+TEST(FileIO, FileShareMode_Read_Write) {
+  FileShareModeTest(ReadOrWrite::kRead, ReadOrWrite::kWrite);
+}
+
+TEST(FileIO, FileShareMode_Write_Read) {
+  FileShareModeTest(ReadOrWrite::kWrite, ReadOrWrite::kRead);
+}
+
+TEST(FileIO, FileShareMode_Write_Write) {
+  FileShareModeTest(ReadOrWrite::kWrite, ReadOrWrite::kWrite);
+}
+
+TEST(FileIO, MultipleSharedLocks) {
+  ScopedTempDir temp_dir;
+  base::FilePath shared_file =
+      temp_dir.path().Append(FILE_PATH_LITERAL("file_to_lock"));
+
+  {
+    // Create an empty file to lock.
+    ScopedFileHandle create(
+        LoggingOpenFileForWrite(shared_file,
+                                FileWriteMode::kCreateOrFail,
+                                FilePermissions::kOwnerOnly));
+  }
+
+  auto handle1 = ScopedFileHandle(LoggingOpenFileForRead(shared_file));
+  ASSERT_NE(handle1, kInvalidFileHandle);
+  EXPECT_TRUE(LoggingLockFile(handle1.get(), FileLocking::kShared));
+
+  auto handle2 = ScopedFileHandle(LoggingOpenFileForRead(shared_file));
+  ASSERT_NE(handle1, kInvalidFileHandle);
+  EXPECT_TRUE(LoggingLockFile(handle2.get(), FileLocking::kShared));
+
+  EXPECT_TRUE(LoggingUnlockFile(handle1.get()));
+  EXPECT_TRUE(LoggingUnlockFile(handle2.get()));
+}
+
+class LockingTestThread : public Thread {
+ public:
+  LockingTestThread()
+      : file_(), lock_type_(), iterations_(), actual_iterations_() {}
+
+  void Init(FileHandle file,
+            FileLocking lock_type,
+            int iterations,
+            base::subtle::Atomic32* actual_iterations) {
+    ASSERT_NE(file, kInvalidFileHandle);
+    file_ = ScopedFileHandle(file);
+    lock_type_ = lock_type;
+    iterations_ = iterations;
+    actual_iterations_ = actual_iterations;
+  }
+
+ private:
+  void ThreadMain() override {
+    for (int i = 0; i < iterations_; ++i) {
+      EXPECT_TRUE(LoggingLockFile(file_.get(), lock_type_));
+      base::subtle::NoBarrier_AtomicIncrement(actual_iterations_, 1);
+      EXPECT_TRUE(LoggingUnlockFile(file_.get()));
+    }
+  }
+
+  ScopedFileHandle file_;
+  FileLocking lock_type_;
+  int iterations_;
+  base::subtle::Atomic32* actual_iterations_;
+
+  DISALLOW_COPY_AND_ASSIGN(LockingTestThread);
+};
+
+void LockingTest(FileLocking main_lock, FileLocking other_locks) {
+  ScopedTempDir temp_dir;
+  base::FilePath shared_file =
+      temp_dir.path().Append(FILE_PATH_LITERAL("file_to_lock"));
+
+  {
+    // Create an empty file to lock.
+    ScopedFileHandle create(
+        LoggingOpenFileForWrite(shared_file,
+                                FileWriteMode::kCreateOrFail,
+                                FilePermissions::kOwnerOnly));
+  }
+
+  auto initial = ScopedFileHandle(
+      (main_lock == FileLocking::kShared)
+          ? LoggingOpenFileForRead(shared_file)
+          : LoggingOpenFileForWrite(shared_file,
+                                    FileWriteMode::kReuseOrCreate,
+                                    FilePermissions::kOwnerOnly));
+  ASSERT_NE(initial, kInvalidFileHandle);
+  ASSERT_TRUE(LoggingLockFile(initial.get(), main_lock));
+
+  base::subtle::Atomic32 actual_iterations = 0;
+
+  LockingTestThread threads[20];
+  int expected_iterations = 0;
+  for (size_t index = 0; index < arraysize(threads); ++index) {
+    int iterations_for_this_thread = static_cast<int>(index * 10);
+    threads[index].Init(
+        (other_locks == FileLocking::kShared)
+            ? LoggingOpenFileForRead(shared_file)
+            : LoggingOpenFileForWrite(shared_file,
+                                      FileWriteMode::kReuseOrCreate,
+                                      FilePermissions::kOwnerOnly),
+        other_locks,
+        iterations_for_this_thread,
+        &actual_iterations);
+    expected_iterations += iterations_for_this_thread;
+
+    ASSERT_NO_FATAL_FAILURE(threads[index].Start());
+  }
+
+  base::subtle::Atomic32 result =
+      base::subtle::Release_Load(&actual_iterations);
+  EXPECT_EQ(0, result);
+
+  ASSERT_TRUE(LoggingUnlockFile(initial.get()));
+
+  for (auto& t : threads)
+    t.Join();
+
+  result = base::subtle::Release_Load(&actual_iterations);
+  EXPECT_EQ(expected_iterations, result);
+}
+
+TEST(FileIO, ExclusiveVsExclusives) {
+  LockingTest(FileLocking::kExclusive, FileLocking::kExclusive);
+}
+
+TEST(FileIO, ExclusiveVsShareds) {
+  LockingTest(FileLocking::kExclusive, FileLocking::kShared);
+}
+
+TEST(FileIO, SharedVsExclusives) {
+  LockingTest(FileLocking::kShared, FileLocking::kExclusive);
+}
+
+}  // namespace
+}  // namespace test
+}  // namespace crashpad

util/file/file_io_win.cc

@@ -80,8 +80,13 @@ ssize_t WriteFile(FileHandle file, const void* buffer, size_t size) {
 }
 
 FileHandle LoggingOpenFileForRead(const base::FilePath& path) {
-  HANDLE file = CreateFile(path.value().c_str(), GENERIC_READ, FILE_SHARE_READ,
+  HANDLE file = CreateFile(path.value().c_str(),
-                           nullptr, OPEN_EXISTING, 0, nullptr);
+                           GENERIC_READ,
+                           FILE_SHARE_READ | FILE_SHARE_WRITE,
+                           nullptr,
+                           OPEN_EXISTING,
+                           0,
+                           nullptr);
   PLOG_IF(ERROR, file == INVALID_HANDLE_VALUE) << "CreateFile "
                                                << path.value().c_str();
   return file;
@@ -102,13 +107,45 @@ FileHandle LoggingOpenFileForWrite(const base::FilePath& path,
       disposition = CREATE_NEW;
       break;
   }
-  HANDLE file = CreateFile(path.value().c_str(), GENERIC_WRITE, 0, nullptr,
+  HANDLE file = CreateFile(path.value().c_str(),
-                           disposition, FILE_ATTRIBUTE_NORMAL, nullptr);
+                           GENERIC_WRITE,
+                           FILE_SHARE_READ | FILE_SHARE_WRITE,
+                           nullptr,
+                           disposition,
+                           FILE_ATTRIBUTE_NORMAL,
+                           nullptr);
   PLOG_IF(ERROR, file == INVALID_HANDLE_VALUE) << "CreateFile "
                                                << path.value().c_str();
   return file;
 }
 
+bool LoggingLockFile(FileHandle file, FileLocking locking) {
+  DWORD flags =
+      (locking == FileLocking::kExclusive) ? LOCKFILE_EXCLUSIVE_LOCK : 0;
+
+  // 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)) {
+    PLOG(ERROR) << "LockFileEx";
+    return false;
+  }
+  return true;
+}
+
+bool LoggingUnlockFile(FileHandle file) {
+  // 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 (!UnlockFileEx(file, 0, MAXDWORD, MAXDWORD, &overlapped)) {
+    PLOG(ERROR) << "UnlockFileEx";
+    return false;
+  }
+  return true;
+}
+
 FileOffset LoggingSeekFile(FileHandle file, FileOffset offset, int whence) {
   DWORD method = 0;
   switch (whence) {

util/test/errors.cc

@@ -16,13 +16,15 @@
 
 #include <errno.h>
 
-#include "build/build_config.h"
+#include "base/logging.h"
 #include "base/strings/stringprintf.h"
+#include "build/build_config.h"
 
 #if defined(OS_POSIX)
 #include "base/safe_strerror_posix.h"
 #elif defined(OS_WIN)
 #include <string.h>
+#include <windows.h>
 #endif
 
 namespace crashpad {
@@ -47,5 +49,15 @@ std::string ErrnoMessage(const std::string& base) {
   return ErrnoMessage(errno, base);
 }
 
+#if defined(OS_WIN)
+std::string ErrorMessage(const std::string& base) {
+  return base::StringPrintf(
+      "%s%s%s",
+      base.c_str(),
+      base.empty() ? "" : ": ",
+      logging::SystemErrorCodeToString(GetLastError()).c_str());
+}
+#endif
+
 }  // namespace test
 }  // namespace crashpad

util/test/errors.h

@@ -17,6 +17,8 @@
 
 #include <string>
 
+#include "build/build_config.h"
+
 namespace crashpad {
 namespace test {
 
@@ -65,6 +67,15 @@ std::string ErrnoMessage(int err, const std::string& base = std::string());
 //!     a colon.
 std::string ErrnoMessage(const std::string& base = std::string());
 
+#if defined(OS_WIN) || DOXYGEN
+//! \brief Formats an error message using `GetLastError()`.
+//!
+//! The returned string will combine the \a base string, if supplied, with a
+//! a textual and numeric description of the error. The format is the same as
+//! the `PLOG()` formatting in base.
+std::string ErrorMessage(const std::string& base = std::string());
+#endif
+
 }  // namespace test
 }  // namespace crashpad
 

util/test/thread.cc

@@ -0,0 +1,30 @@
+// Copyright 2015 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "util/test/thread.h"
+
+#include "gtest/gtest.h"
+
+namespace crashpad {
+namespace test {
+
+Thread::Thread() : platform_thread_(0) {
+}
+
+Thread::~Thread() {
+  EXPECT_FALSE(platform_thread_);
+}
+
+}  // namespace test
+}  // namespace crashpad

util/test/thread.h

@@ -0,0 +1,70 @@
+// Copyright 2015 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef CRASHPAD_UTIL_TEST_THREAD_H_
+#define CRASHPAD_UTIL_TEST_THREAD_H_
+
+#include "base/basictypes.h"
+#include "build/build_config.h"
+
+#if defined(OS_POSIX)
+#include <pthread.h>
+#elif defined(OS_WIN)
+#include <windows.h>
+#endif  // OS_POSIX
+
+namespace crashpad {
+namespace test {
+
+//! \brief Basic thread abstraction for testing. Users should derive from this
+//!     class and implement ThreadMain().
+class Thread {
+ public:
+  Thread();
+  virtual ~Thread();
+
+  //! \brief Create a platform thread, and run ThreadMain() on that thread. Must
+  //!     be paired with a call to Join().
+  void Start();
+
+  //! \brief Block until ThreadMain() exits. This may be called from any thread.
+  //!     Must paired with a call to Start().
+  void Join();
+
+ private:
+  //! \brief The entry point to be overridden to implement the test-specific
+  //!     functionality.
+  virtual void ThreadMain() = 0;
+
+  static
+#if defined(OS_POSIX)
+      void*
+#elif defined(OS_WIN)
+      DWORD WINAPI
+#endif  // OS_POSIX
+      ThreadEntryThunk(void* argument);
+
+#if defined(OS_POSIX)
+  pthread_t platform_thread_;
+#elif defined(OS_WIN)
+  HANDLE platform_thread_;
+#endif
+
+  DISALLOW_COPY_AND_ASSIGN(Thread);
+};
+
+}  // namespace test
+}  // namespace crashpad
+
+#endif  // CRASHPAD_UTIL_TEST_THREAD_H_

util/test/thread_posix.cc

@@ -0,0 +1,44 @@
+// Copyright 2015 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "util/test/thread.h"
+
+#include "gtest/gtest.h"
+#include "util/test/errors.h"
+
+namespace crashpad {
+namespace test {
+
+void Thread::Start() {
+  ASSERT_FALSE(platform_thread_);
+  int rv = pthread_create(&platform_thread_, nullptr, ThreadEntryThunk, this);
+  ASSERT_EQ(0, rv) << ErrnoMessage(rv, "pthread_create");
+}
+
+void Thread::Join() {
+  ASSERT_TRUE(platform_thread_);
+  int rv = pthread_join(platform_thread_, nullptr);
+  EXPECT_EQ(0, rv) << ErrnoMessage(rv, "pthread_join");
+  platform_thread_ = 0;
+}
+
+// static
+void* Thread::ThreadEntryThunk(void* argument) {
+  Thread* self = reinterpret_cast<Thread*>(argument);
+  self->ThreadMain();
+  return nullptr;
+}
+
+}  // namespace test
+}  // namespace crashpad

util/test/thread_win.cc

@@ -0,0 +1,45 @@
+// Copyright 2015 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "util/test/thread.h"
+
+#include "gtest/gtest.h"
+#include "util/test/errors.h"
+
+namespace crashpad {
+namespace test {
+
+void Thread::Start() {
+  ASSERT_FALSE(platform_thread_);
+  platform_thread_ =
+      CreateThread(nullptr, 0, ThreadEntryThunk, this, 0, nullptr);
+  ASSERT_TRUE(platform_thread_) << ErrorMessage("CreateThread");
+}
+
+void Thread::Join() {
+  ASSERT_FALSE(platform_thread_);
+  DWORD result = WaitForSingleObject(platform_thread_, INFINITE);
+  EXPECT_EQ(WAIT_OBJECT_0, result) << ErrorMessage("WaitForSingleObject");
+  platform_thread_ = 0;
+}
+
+// static
+DWORD WINAPI Thread::ThreadEntryThunk(void* argument) {
+  Thread* self = reinterpret_cast<Thread*>(argument);
+  self->ThreadMain();
+  return 0;
+}
+
+}  // namespace test
+}  // namespace crashpad

util/util.gyp

@@ -249,6 +249,10 @@
         'test/scoped_temp_dir.h',
         'test/scoped_temp_dir_posix.cc',
         'test/scoped_temp_dir_win.cc',
+        'test/thread.cc',
+        'test/thread.h',
+        'test/thread_posix.cc',
+        'test/thread_win.cc',
       ],
       'conditions': [
         ['OS=="mac"', {
@@ -277,6 +281,7 @@
         '..',
       ],
       'sources': [
+        'file/file_io_test.cc',
         'file/string_file_test.cc',
         'mac/checked_mach_address_range_test.cc',
         'mac/launchd_test.mm',