From bae8707d6444605b9fd7b86a3823d754f6d9cd05 Mon Sep 17 00:00:00 2001
From: Sebastian Messmer <messmer@cryfs.org>
Date: Wed, 26 Dec 2018 15:12:01 +0100
Subject: [PATCH] Fix deadlock

---
 src/cryfs/filesystem/fsblobstore/DirBlob.cpp | 55 ++++++++++++--------
 src/cryfs/filesystem/fsblobstore/DirBlob.h   |  3 +-
 2 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/src/cryfs/filesystem/fsblobstore/DirBlob.cpp b/src/cryfs/filesystem/fsblobstore/DirBlob.cpp
index 2f92cdb8..206ede68 100644
--- a/src/cryfs/filesystem/fsblobstore/DirBlob.cpp
+++ b/src/cryfs/filesystem/fsblobstore/DirBlob.cpp
@@ -27,18 +27,18 @@ namespace fsblobstore {
 constexpr fspp::num_bytes_t DirBlob::DIR_LSTAT_SIZE;
 
 DirBlob::DirBlob(unique_ref<Blob> blob, std::function<fspp::num_bytes_t (const blockstore::BlockId&)> getLstatSize) :
-    FsBlob(std::move(blob)), _getLstatSize(getLstatSize), _entries(), _mutex(), _changed(false) {
+    FsBlob(std::move(blob)), _getLstatSize(getLstatSize), _getLstatSizeMutex(), _entries(), _entriesAndChangedMutex(), _changed(false) {
   ASSERT(baseBlob().blobType() == FsBlobView::BlobType::DIR, "Loaded blob is not a directory");
   _readEntriesFromBlob();
 }
 
 DirBlob::~DirBlob() {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   _writeEntriesToBlob();
 }
 
 void DirBlob::flush() {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   _writeEntriesToBlob();
   baseBlob().flush();
 }
@@ -64,17 +64,17 @@ void DirBlob::_readEntriesFromBlob() {
 }
 
 void DirBlob::AddChildDir(const std::string &name, const BlockId &blobId, fspp::mode_t mode, fspp::uid_t uid, fspp::gid_t gid, timespec lastAccessTime, timespec lastModificationTime) {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   _addChild(name, blobId, fspp::Dir::EntryType::DIR, mode, uid, gid, lastAccessTime, lastModificationTime);
 }
 
 void DirBlob::AddChildFile(const std::string &name, const BlockId &blobId, fspp::mode_t mode, fspp::uid_t uid, fspp::gid_t gid, timespec lastAccessTime, timespec lastModificationTime) {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   _addChild(name, blobId, fspp::Dir::EntryType::FILE, mode, uid, gid, lastAccessTime, lastModificationTime);
 }
 
 void DirBlob::AddChildSymlink(const std::string &name, const blockstore::BlockId &blobId, fspp::uid_t uid, fspp::gid_t gid, timespec lastAccessTime, timespec lastModificationTime) {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   auto mode = fspp::mode_t().addSymlinkFlag()
           .addUserReadFlag().addUserWriteFlag().addUserExecFlag()
           .addGroupReadFlag().addGroupWriteFlag().addGroupExecFlag()
@@ -91,41 +91,41 @@ void DirBlob::_addChild(const std::string &name, const BlockId &blobId,
 void DirBlob::AddOrOverwriteChild(const std::string &name, const BlockId &blobId, fspp::Dir::EntryType entryType,
                                   fspp::mode_t mode, fspp::uid_t uid, fspp::gid_t gid, timespec lastAccessTime, timespec lastModificationTime,
                                   std::function<void (const blockstore::BlockId &blockId)> onOverwritten) {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   _entries.addOrOverwrite(name, blobId, entryType, mode, uid, gid, lastAccessTime, lastModificationTime, onOverwritten);
   _changed = true;
 }
 
 void DirBlob::RenameChild(const blockstore::BlockId &blockId, const std::string &newName, std::function<void (const blockstore::BlockId &blockId)> onOverwritten) {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   _entries.rename(blockId, newName, onOverwritten);
   _changed = true;
 }
 
 boost::optional<const DirEntry&> DirBlob::GetChild(const string &name) const {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   return _entries.get(name);
 }
 
 boost::optional<const DirEntry&> DirBlob::GetChild(const BlockId &blockId) const {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   return _entries.get(blockId);
 }
 
 void DirBlob::RemoveChild(const string &name) {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   _entries.remove(name);
   _changed = true;
 }
 
 void DirBlob::RemoveChild(const BlockId &blockId) {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   _entries.remove(blockId);
   _changed = true;
 }
 
 void DirBlob::AppendChildrenTo(vector<fspp::Dir::Entry> *result) const {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   result->reserve(result->size() + _entries.size());
   for (const auto &entry : _entries) {
     result->emplace_back(entry.type(), entry.name());
@@ -137,7 +137,18 @@ fspp::num_bytes_t DirBlob::lstat_size() const {
 }
 
 fspp::Node::stat_info DirBlob::statChild(const BlockId &blockId) const {
-  return statChildWithKnownSize(blockId, _getLstatSize(blockId));
+  std::unique_lock<std::mutex> lock(_getLstatSizeMutex);
+  auto lstatSizeGetter = _getLstatSize;
+
+  // The following unlock is important to avoid deadlock.
+  // ParallelAccessFsBlobStore::load() causes a call to DirBlob::setLstatSizeGetter,
+  // so their lock ordering first locks the ParallelAccessStore::_mutex, then the DirBlob::_getLstatSizeMutex.
+  // this requires us to free DirBlob::_getLstatSizeMutex before calling into lstatSizeGetter(), because
+  // lstatSizeGetter can call ParallelAccessFsBlobStore::load().
+  lock.unlock();
+
+  auto lstatSize = lstatSizeGetter(blockId);
+  return statChildWithKnownSize(blockId, lstatSize);
 }
 
 fspp::Node::stat_info DirBlob::statChildWithKnownSize(const BlockId &blockId, fspp::num_bytes_t size) const {
@@ -163,44 +174,44 @@ fspp::Node::stat_info DirBlob::statChildWithKnownSize(const BlockId &blockId, fs
 }
 
 void DirBlob::updateAccessTimestampForChild(const BlockId &blockId, TimestampUpdateBehavior timestampUpdateBehavior) {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   if (_entries.updateAccessTimestampForChild(blockId, timestampUpdateBehavior)) {
     _changed = true;
   }
 }
 
 void DirBlob::updateModificationTimestampForChild(const BlockId &blockId) {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   _entries.updateModificationTimestampForChild(blockId);
   _changed = true;
 }
 
 void DirBlob::chmodChild(const BlockId &blockId, fspp::mode_t mode) {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   _entries.setMode(blockId, mode);
   _changed = true;
 }
 
 void DirBlob::chownChild(const BlockId &blockId, fspp::uid_t uid, fspp::gid_t gid) {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   if(_entries.setUidGid(blockId, uid, gid)) {
     _changed = true;
   }
 }
 
 void DirBlob::utimensChild(const BlockId &blockId, timespec lastAccessTime, timespec lastModificationTime) {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   _entries.setAccessTimes(blockId, lastAccessTime, lastModificationTime);
   _changed = true;
 }
 
 void DirBlob::setLstatSizeGetter(std::function<fspp::num_bytes_t(const blockstore::BlockId&)> getLstatSize) {
-    std::unique_lock<std::mutex> lock(_mutex);
-    _getLstatSize = getLstatSize;
+    std::lock_guard<std::mutex> lock(_getLstatSizeMutex);
+    _getLstatSize = std::move(getLstatSize);
 }
 
 cpputils::unique_ref<blobstore::Blob> DirBlob::releaseBaseBlob() {
-  std::unique_lock<std::mutex> lock(_mutex);
+  std::unique_lock<std::mutex> lock(_entriesAndChangedMutex);
   _writeEntriesToBlob();
   return FsBlob::releaseBaseBlob();
 }
diff --git a/src/cryfs/filesystem/fsblobstore/DirBlob.h b/src/cryfs/filesystem/fsblobstore/DirBlob.h
index 1546059c..822fb7f4 100644
--- a/src/cryfs/filesystem/fsblobstore/DirBlob.h
+++ b/src/cryfs/filesystem/fsblobstore/DirBlob.h
@@ -83,8 +83,9 @@ namespace cryfs {
             cpputils::unique_ref<blobstore::Blob> releaseBaseBlob() override;
 
             std::function<fspp::num_bytes_t (const blockstore::BlockId&)> _getLstatSize;
+            mutable std::mutex _getLstatSizeMutex;
             DirEntryList _entries;
-            mutable std::mutex _mutex;
+            mutable std::mutex _entriesAndChangedMutex;
             bool _changed;
 
             DISALLOW_COPY_AND_ASSIGN(DirBlob);