Merge branch 'feature/either3' into feature/library_intermediate

.circleci/config.yml

@@ -175,11 +175,11 @@ references:
           cd cmake
           ./test/gitversion/gitversion-test
           if [ ! "$DISABLE_BROKEN_TSAN_TESTS" = true ] ; then ./test/cpp-utils/cpp-utils-test ; fi
-          if [ ! "$DISABLE_BROKEN_TSAN_TESTS" = true ] && [ ! "$DISABLE_BROKEN_ASAN_TESTS" = true ] ; then ./test/fspp/fspp-test ; fi
+          if [ ! "$DISABLE_BROKEN_ASAN_TESTS" = true ] ; then ./test/fspp/fspp-test ; fi
           ./test/parallelaccessstore/parallelaccessstore-test
           ./test/blockstore/blockstore-test
           ./test/blobstore/blobstore-test
-          if [ ! "$DISABLE_BROKEN_TSAN_TESTS" = true ] ; then ./test/cryfs/cryfs-test ; fi
+          ./test/cryfs/cryfs-test
           if [ ! "$DISABLE_BROKEN_TSAN_TESTS" = true ] ; then ./test/cryfs-cli/cryfs-cli-test ; fi
         fi
   job_definition: &job_definition
@@ -454,6 +454,7 @@ jobs:
       CXX: clang++-7
       BUILD_TOOLSET: clang
       APT_COMPILER_PACKAGE: clang-7
+      OMP_NUM_THREADS: "1"
       CXXFLAGS: "-O2 -fsanitize=thread -fno-omit-frame-pointer"
       BUILD_TYPE: "Debug"
       DISABLE_BROKEN_TSAN_TESTS: true

ChangeLog.txt

@@ -18,6 +18,12 @@ Fixed bugs:
 * On Mac OS X, Finder shows the correct name for the mount directory
 
 
+Version 0.9.10 (unreleased)
+--------------
+Fixed bugs:
+* Fixed occasional deadlock (https://github.com/cryfs/cryfs/issues/64)
+
+
 Version 0.9.9
 --------------
 Improvements:

src/cpp-utils/either.h

@@ -33,7 +33,7 @@ namespace cpputils {
             }
         }
 
-        either(either<Left, Right> &&rhs) noexcept(noexcept(_construct_left(std::move(rhs._left))) && noexcept(_construct_right(std::move(rhs._right))))
+        either(either<Left, Right> &&rhs) noexcept(noexcept(std::declval<either<Left, Right>>()._construct_left(std::move(rhs._left))) && noexcept(std::declval<either<Left, Right>>()._construct_right(std::move(rhs._right))))
                 : _side(rhs._side) {
             if(_side == Side::left) {
                 _construct_left(std::move(rhs._left));  // NOLINT(cppcoreguidelines-pro-type-union-access)
@@ -47,7 +47,7 @@ namespace cpputils {
         }
 
         //TODO Try allowing copy-assignment when Left/Right types are std::is_convertible
-        either<Left, Right> &operator=(const either<Left, Right> &rhs) noexcept(noexcept(_construct_left(rhs._left)) && noexcept(_construct_right(rhs._right))) {
+        either<Left, Right> &operator=(const either<Left, Right> &rhs) noexcept(noexcept(std::declval<either<Left, Right>>()._construct_left(rhs._left)) && noexcept(std::declval<either<Left, Right>>()._construct_right(rhs._right))) {
             _destruct();
             _side = rhs._side;
             if (_side == Side::left) {
@@ -58,7 +58,7 @@ namespace cpputils {
             return *this;
         }
 
-        either<Left, Right> &operator=(either<Left, Right> &&rhs) noexcept(noexcept(_construct_left(std::move(rhs._left))) && noexcept(_construct_right(std::move(rhs._right)))) {
+        either<Left, Right> &operator=(either<Left, Right> &&rhs) noexcept(noexcept(std::declval<either<Left, Right>>()._construct_left(std::move(rhs._left))) && noexcept(std::declval<either<Left, Right>>()._construct_right(std::move(rhs._right)))) {
             _destruct();
             _side = rhs._side;
             if (_side == Side::left) {

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();
 }

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);