From 648263d0623c01d0bb5985dde5855267479673ce Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Wed, 26 Dec 2018 16:46:59 +0100 Subject: [PATCH 1/5] TSAN and omp don't work together, reduce omp thread count in tsan to 1 --- .circleci/config.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index f9194ef6..8e8f484e 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -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 From 2bae1281c66672b4b4e285388c547b0636e53a1b Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Wed, 26 Dec 2018 15:12:01 +0100 Subject: [PATCH 2/5] 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, std::function 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 lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _writeEntriesToBlob(); } void DirBlob::flush() { - std::unique_lock lock(_mutex); + std::unique_lock 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 lock(_mutex); + std::unique_lock 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 lock(_mutex); + std::unique_lock 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 lock(_mutex); + std::unique_lock 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 onOverwritten) { - std::unique_lock lock(_mutex); + std::unique_lock 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 onOverwritten) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _entries.rename(blockId, newName, onOverwritten); _changed = true; } boost::optional DirBlob::GetChild(const string &name) const { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); return _entries.get(name); } boost::optional DirBlob::GetChild(const BlockId &blockId) const { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); return _entries.get(blockId); } void DirBlob::RemoveChild(const string &name) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _entries.remove(name); _changed = true; } void DirBlob::RemoveChild(const BlockId &blockId) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _entries.remove(blockId); _changed = true; } void DirBlob::AppendChildrenTo(vector *result) const { - std::unique_lock lock(_mutex); + std::unique_lock 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 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 lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); if (_entries.updateAccessTimestampForChild(blockId, timestampUpdateBehavior)) { _changed = true; } } void DirBlob::updateModificationTimestampForChild(const BlockId &blockId) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _entries.updateModificationTimestampForChild(blockId); _changed = true; } void DirBlob::chmodChild(const BlockId &blockId, fspp::mode_t mode) { - std::unique_lock lock(_mutex); + std::unique_lock 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 lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); if(_entries.setUidGid(blockId, uid, gid)) { _changed = true; } } void DirBlob::utimensChild(const BlockId &blockId, timespec lastAccessTime, timespec lastModificationTime) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _entries.setAccessTimes(blockId, lastAccessTime, lastModificationTime); _changed = true; } void DirBlob::setLstatSizeGetter(std::function getLstatSize) { - std::unique_lock lock(_mutex); - _getLstatSize = getLstatSize; + std::lock_guard lock(_getLstatSizeMutex); + _getLstatSize = std::move(getLstatSize); } cpputils::unique_ref DirBlob::releaseBaseBlob() { - std::unique_lock lock(_mutex); + std::unique_lock 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 releaseBaseBlob() override; std::function _getLstatSize; + mutable std::mutex _getLstatSizeMutex; DirEntryList _entries; - mutable std::mutex _mutex; + mutable std::mutex _entriesAndChangedMutex; bool _changed; DISALLOW_COPY_AND_ASSIGN(DirBlob); From 50509f84f1885f2eba522a866b7eabf6082f1232 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Thu, 27 Dec 2018 10:43:09 +0100 Subject: [PATCH 3/5] Update changelog --- ChangeLog.txt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ChangeLog.txt b/ChangeLog.txt index a63a8c29..24dc6d59 100644 --- a/ChangeLog.txt +++ b/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: From e8d791c9cfdd7c5c61593870a60784b0a1a12872 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Fri, 28 Dec 2018 09:36:48 +0100 Subject: [PATCH 4/5] Enable TSAN for more builds --- .circleci/config.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 8e8f484e..4513a0b1 100644 --- a/.circleci/config.yml +++ b/.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 From 6663ffd0367cd6195b170cc362f0b4f4556cb42e Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Fri, 28 Dec 2018 09:47:22 +0100 Subject: [PATCH 5/5] fix --- src/cpp-utils/either.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cpp-utils/either.h b/src/cpp-utils/either.h index 45cb2049..cc7a2306 100644 --- a/src/cpp-utils/either.h +++ b/src/cpp-utils/either.h @@ -33,7 +33,7 @@ namespace cpputils { } } - either(either &&rhs) noexcept(noexcept(_construct_left(std::move(rhs._left))) && noexcept(_construct_right(std::move(rhs._right)))) + either(either &&rhs) noexcept(noexcept(std::declval>()._construct_left(std::move(rhs._left))) && noexcept(std::declval>()._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 &operator=(const either &rhs) noexcept(noexcept(_construct_left(rhs._left)) && noexcept(_construct_right(rhs._right))) { + either &operator=(const either &rhs) noexcept(noexcept(std::declval>()._construct_left(rhs._left)) && noexcept(std::declval>()._construct_right(rhs._right))) { _destruct(); _side = rhs._side; if (_side == Side::left) { @@ -58,7 +58,7 @@ namespace cpputils { return *this; } - either &operator=(either &&rhs) noexcept(noexcept(_construct_left(std::move(rhs._left))) && noexcept(_construct_right(std::move(rhs._right)))) { + either &operator=(either &&rhs) noexcept(noexcept(std::declval>()._construct_left(std::move(rhs._left))) && noexcept(std::declval>()._construct_right(std::move(rhs._right)))) { _destruct(); _side = rhs._side; if (_side == Side::left) {