Expose PlatformFileLock/Unlock, fix locking in Chromium's leveldb Env

The base::CreatePlatformFile() flags PLATFORM_FILE_EXCLUSIVE_READ/WRITE
are ineffective on existing files on POSIX, and can't be used to
implement the locking scheme required by leveldb.

Add PlatformFileLock/Unlock which have the correct semantics, and
use those in ChromiumEnv. Also, crib code from leveldb's env_posix.cc
to handle in-process exclusivity which is not guaranteed by
file locks.

BUG=245471

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@228036 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 155 insertions, 9 deletions

diff --git a/base/platform_file.h b/base/platform_file.h
index 3d69ae7..66e5f96 100644
--- a/base/platform_file.h
+++ b/base/platform_file.h
@@ -24,6 +24,8 @@ namespace base {
 // or creating a file.
 // PLATFORM_FILE_(WRITE|APPEND) are mutually exclusive. This is so that APPEND
 // behavior will be consistent with O_APPEND on POSIX.
+// PLATFORM_FILE_EXCLUSIVE_(READ|WRITE) only grant exclusive access to the file
+// on creation on POSIX; for existing files, consider using LockPlatformFile().
 enum PlatformFileFlags {
   PLATFORM_FILE_OPEN = 1 << 0,             // Opens a file, only if it exists.
   PLATFORM_FILE_CREATE = 1 << 1,           // Creates a new file, only if it
@@ -210,6 +212,31 @@ BASE_EXPORT bool TouchPlatformFile(PlatformFile file,
 // Returns some information for the given file.
 BASE_EXPORT bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info);
 
+// Attempts to take an exclusive write lock on the file. Returns immediately
+// (i.e. does not wait for another process to unlock the file). If the lock
+// was obtained, the result will be PLATFORM_FILE_OK. A lock only guarantees
+// that other processes may not also take a lock on the same file with the
+// same API - it may still be opened, renamed, unlinked, etc.
+//
+// Common semantics:
+//  * Locks are held by processes, but not inherited by child processes.
+//  * Locks are released by the OS on file handle close or process termination.
+//  * Locks are reliable only on local filesystems.
+//  * Duplicated file handles may also write to locked files.
+// Windows-specific semantics:
+//  * Locks are mandatory for read/write APIs, advisory for mapping APIs.
+//  * Within a process, locking the same file (by the same or new handle)
+//    will fail.
+// POSIX-specific semantics:
+//  * Locks are advisory only.
+//  * Within a process, locking the same file (by the same or new handle)
+//    will succeed.
+//  * Closing any descriptor on a given file releases the lock.
+BASE_EXPORT PlatformFileError LockPlatformFile(PlatformFile file);
+
+// Unlock a file previously locked with LockPlatformFile.
+BASE_EXPORT PlatformFileError UnlockPlatformFile(PlatformFile file);
+
 // Use this class to pass ownership of a PlatformFile to a receiver that may or
 // may not want to accept it.  This class does not own the storage for the
 // PlatformFile.
diff --git a/base/platform_file_posix.cc b/base/platform_file_posix.cc
index 056577c..7da7ee0 100644
--- a/base/platform_file_posix.cc
+++ b/base/platform_file_posix.cc
@@ -83,6 +83,17 @@ static int CallFutimes(PlatformFile file, const struct timeval times[2]) {
   return futimes(file, times);
 #endif
 }
+
+static PlatformFileError CallFctnlFlock(PlatformFile file, bool do_lock) {
+  struct flock lock;
+  lock.l_type = F_WRLCK;
+  lock.l_whence = SEEK_SET;
+  lock.l_start = 0;
+  lock.l_len = 0;  // Lock entire file.
+  if (HANDLE_EINTR(fcntl(file, do_lock ? F_SETLK : F_UNLCK, &lock)) == -1)
+    return ErrnoToPlatformFileError(errno);
+  return PLATFORM_FILE_OK;
+}
 #else  // defined(OS_NACL)
 // TODO(bbudge) Remove DoPread, DoPwrite when NaCl implements pread, pwrite.
 static int DoPread(PlatformFile file, char* data, int size, int64 offset) {
@@ -117,6 +128,11 @@ static int CallFutimes(PlatformFile file, const struct timeval times[2]) {
   NOTIMPLEMENTED();  // NaCl doesn't implement futimes.
   return 0;
 }
+
+static PlatformFileError CallFctnlFlock(PlatformFile file, bool do_lock) {
+  NOTIMPLEMENTED();  // NaCl doesn't implement flock struct.
+  return PLATFORM_FILE_ERROR_INVALID_OPERATION;
+}
 #endif  // defined(OS_NACL)
 
 }  // namespace
@@ -426,6 +442,14 @@ bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) {
   return true;
 }
 
+PlatformFileError LockPlatformFile(PlatformFile file) {
+  return CallFctnlFlock(file, true);
+}
+
+PlatformFileError UnlockPlatformFile(PlatformFile file) {
+  return CallFctnlFlock(file, false);
+}
+
 PlatformFileError ErrnoToPlatformFileError(int saved_errno) {
   switch (saved_errno) {
     case EACCES:
diff --git a/base/platform_file_win.cc b/base/platform_file_win.cc
index c5b49fa..7765d27 100644
--- a/base/platform_file_win.cc
+++ b/base/platform_file_win.cc
@@ -262,6 +262,20 @@ bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) {
   return true;
 }
 
+PlatformFileError LockPlatformFile(PlatformFile file) {
+  BOOL result = LockFile(file, 0, 0, MAXDWORD, MAXDWORD);
+  if (!result)
+    return LastErrorToPlatformFileError(GetLastError());
+  return PLATFORM_FILE_OK;
+}
+
+PlatformFileError UnlockPlatformFile(PlatformFile file) {
+  BOOL result = UnlockFile(file, 0, 0, MAXDWORD, MAXDWORD);
+  if (!result)
+    return LastErrorToPlatformFileError(GetLastError());
+  return PLATFORM_FILE_OK;
+}
+
 PlatformFileError LastErrorToPlatformFileError(DWORD last_error) {
   switch (last_error) {
     case ERROR_SHARING_VIOLATION:
diff --git a/content/browser/indexed_db/leveldb/leveldb_unittest.cc b/content/browser/indexed_db/leveldb/leveldb_unittest.cc
index de506c5..3edcae7 100644
--- a/content/browser/indexed_db/leveldb/leveldb_unittest.cc
+++ b/content/browser/indexed_db/leveldb/leveldb_unittest.cc
@@ -16,6 +16,8 @@
 #include "content/browser/indexed_db/leveldb/leveldb_iterator.h"
 #include "content/browser/indexed_db/leveldb/leveldb_transaction.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/leveldatabase/env_chromium.h"
+#include "third_party/leveldatabase/env_idb.h"
 
 namespace content {
 
@@ -243,6 +245,30 @@ TEST(LevelDBDatabaseTest, TransactionCommitTest) {
   EXPECT_EQ(value3, got_value);
 }
 
+TEST(LevelDB, Locking) {
+  base::ScopedTempDir temp_directory;
+  ASSERT_TRUE(temp_directory.CreateUniqueTempDir());
+
+  leveldb::Env* env = leveldb::IDBEnv();
+  base::FilePath file = temp_directory.path().AppendASCII("LOCK");
+  leveldb::FileLock* lock;
+  leveldb::Status status = env->LockFile(file.AsUTF8Unsafe(), &lock);
+  EXPECT_TRUE(status.ok());
+
+  status = env->UnlockFile(lock);
+  EXPECT_TRUE(status.ok());
+
+  status = env->LockFile(file.AsUTF8Unsafe(), &lock);
+  EXPECT_TRUE(status.ok());
+
+  leveldb::FileLock* lock2;
+  status = env->LockFile(file.AsUTF8Unsafe(), &lock2);
+  EXPECT_FALSE(status.ok());
+
+  status = env->UnlockFile(lock);
+  EXPECT_TRUE(status.ok());
+}
+
 }  // namespace
 
 }  // namespace content
diff --git a/third_party/leveldatabase/README.chromium b/third_party/leveldatabase/README.chromium
index 32670a7..f12a0bf 100644
--- a/third_party/leveldatabase/README.chromium
+++ b/third_party/leveldatabase/README.chromium
@@ -22,3 +22,4 @@ Local Additions:
 * ChromiumEnv wraps low-level I/O calls that may be interrupted with a
   HANDLE_EINTR macro that retries the call.
 * TRACE macros/thread name for chrome://tracing diagnostics
+* Handle in-process exclusive file locks, based on src/util/env_posix.cc
diff --git a/third_party/leveldatabase/env_chromium.cc b/third_party/leveldatabase/env_chromium.cc
index 0a78b48..b297425 100644
--- a/third_party/leveldatabase/env_chromium.cc
+++ b/third_party/leveldatabase/env_chromium.cc
@@ -207,6 +207,7 @@ class ChromiumRandomAccessFile: public RandomAccessFile {
 class ChromiumFileLock : public FileLock {
  public:
   ::base::PlatformFile file_;
+  std::string name_;
 };
 
 class Retrier {
@@ -680,9 +681,7 @@ Status ChromiumEnv::LockFile(const std::string& fname, FileLock** lock) {
   Status result;
   int flags = ::base::PLATFORM_FILE_OPEN_ALWAYS |
               ::base::PLATFORM_FILE_READ |
-              ::base::PLATFORM_FILE_WRITE |
-              ::base::PLATFORM_FILE_EXCLUSIVE_READ |
-              ::base::PLATFORM_FILE_EXCLUSIVE_WRITE;
+              ::base::PLATFORM_FILE_WRITE;
   bool created;
   ::base::PlatformFileError error_code;
   ::base::PlatformFile file;
@@ -711,21 +710,55 @@ Status ChromiumEnv::LockFile(const std::string& fname, FileLock** lock) {
     result = MakeIOError(
         fname, PlatformFileErrorString(error_code), kLockFile, error_code);
     RecordOSError(kLockFile, error_code);
-  } else {
-    ChromiumFileLock* my_lock = new ChromiumFileLock;
-    my_lock->file_ = file;
-    *lock = my_lock;
+    return result;
+  }
+
+  if (!locks_.Insert(fname)) {
+    result = MakeIOError(fname, "Lock file already locked.", kLockFile);
+    ::base::ClosePlatformFile(file);
+    return result;
   }
+
+  Retrier lock_retrier = Retrier(kLockFile, this);
+  do {
+    error_code = ::base::LockPlatformFile(file);
+  } while (error_code != ::base::PLATFORM_FILE_OK &&
+           retrier.ShouldKeepTrying(error_code));
+
+  if (error_code != ::base::PLATFORM_FILE_OK) {
+    ::base::ClosePlatformFile(file);
+    locks_.Remove(fname);
+    result = MakeIOError(
+        fname, PlatformFileErrorString(error_code), kLockFile, error_code);
+    RecordOSError(kLockFile, error_code);
+    return result;
+  }
+
+  ChromiumFileLock* my_lock = new ChromiumFileLock;
+  my_lock->file_ = file;
+  my_lock->name_ = fname;
+  *lock = my_lock;
   return result;
 }
 
 Status ChromiumEnv::UnlockFile(FileLock* lock) {
   ChromiumFileLock* my_lock = reinterpret_cast<ChromiumFileLock*>(lock);
   Status result;
-  if (!::base::ClosePlatformFile(my_lock->file_)) {
-    result = MakeIOError("Could not close lock file.", "", kUnlockFile);
+
+  ::base::PlatformFileError error_code =
+      ::base::UnlockPlatformFile(my_lock->file_);
+  if (error_code != ::base::PLATFORM_FILE_OK) {
+    result =
+        MakeIOError(my_lock->name_, "Could not unlock lock file.", kUnlockFile);
+    RecordOSError(kUnlockFile, error_code);
+    ::base::ClosePlatformFile(my_lock->file_);
+  } else if (!::base::ClosePlatformFile(my_lock->file_)) {
+    result =
+        MakeIOError(my_lock->name_, "Could not close lock file.", kUnlockFile);
     RecordErrorAt(kUnlockFile);
   }
+  bool removed = locks_.Remove(my_lock->name_);
+  DCHECK(removed);
   delete my_lock;
   return result;
 }
diff --git a/third_party/leveldatabase/env_chromium.h b/third_party/leveldatabase/env_chromium.h
index b23969c..7b7b55e 100644
--- a/third_party/leveldatabase/env_chromium.h
+++ b/third_party/leveldatabase/env_chromium.h
@@ -7,6 +7,7 @@
 
 #include <deque>
 #include <map>
+#include <set>
 
 #include "base/metrics/histogram.h"
 #include "base/platform_file.h"
@@ -14,6 +15,8 @@
 #include "leveldb/env.h"
 #include "leveldb/slice.h"
 #include "leveldb/status.h"
+#include "port/port_chromium.h"
+#include "util/mutexlock.h"
 
 namespace leveldb_env {
 
@@ -158,6 +161,23 @@ class ChromiumEnv : public leveldb::Env,
   std::string name_;
 
  private:
+  // File locks may not be exclusive within a process (e.g. on POSIX). Track
+  // locks held by the ChromiumEnv to prevent access within the process.
+  class LockTable {
+   public:
+    bool Insert(const std::string& fname) {
+      leveldb::MutexLock l(&mu_);
+      return locked_files_.insert(fname).second;
+    }
+    bool Remove(const std::string& fname) {
+      leveldb::MutexLock l(&mu_);
+      return locked_files_.erase(fname) == 1;
+    }
+   private:
+    leveldb::port::Mutex mu_;
+    std::set<std::string> locked_files_;
+  };
+
   std::map<std::string, bool> needs_sync_map_;
   base::Lock map_lock_;
 
@@ -198,6 +218,7 @@ class ChromiumEnv : public leveldb::Env,
   };
   typedef std::deque<BGItem> BGQueue;
   BGQueue queue_;
+  LockTable locks_;
 };
 
 }  // namespace leveldb_env