From 2449cc0790af24ae4bc1c5e4e5f0f9328c45c7b5 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Wed, 26 Dec 2018 15:12:01 +0100 Subject: [PATCH 01/10] Fix deadlock --- src/cryfs/filesystem/fsblobstore/DirBlob.cpp | 52 ++++++++++++-------- src/cryfs/filesystem/fsblobstore/DirBlob.h | 3 +- 2 files changed, 33 insertions(+), 22 deletions(-) diff --git a/src/cryfs/filesystem/fsblobstore/DirBlob.cpp b/src/cryfs/filesystem/fsblobstore/DirBlob.cpp index 53b134c4..d2edc5bd 100644 --- a/src/cryfs/filesystem/fsblobstore/DirBlob.cpp +++ b/src/cryfs/filesystem/fsblobstore/DirBlob.cpp @@ -29,18 +29,18 @@ namespace fsblobstore { constexpr off_t DirBlob::DIR_LSTAT_SIZE; DirBlob::DirBlob(FsBlobStore *fsBlobStore, unique_ref blob, std::function getLstatSize) : - FsBlob(std::move(blob)), _fsBlobStore(fsBlobStore), _getLstatSize(getLstatSize), _entries(), _mutex(), _changed(false) { + FsBlob(std::move(blob)), _fsBlobStore(fsBlobStore), _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(); } @@ -66,17 +66,17 @@ void DirBlob::_readEntriesFromBlob() { } void DirBlob::AddChildDir(const std::string &name, const Key &blobKey, mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _addChild(name, blobKey, fspp::Dir::EntryType::DIR, mode, uid, gid, lastAccessTime, lastModificationTime); } void DirBlob::AddChildFile(const std::string &name, const Key &blobKey, mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _addChild(name, blobKey, fspp::Dir::EntryType::FILE, mode, uid, gid, lastAccessTime, lastModificationTime); } void DirBlob::AddChildSymlink(const std::string &name, const blockstore::Key &blobKey, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _addChild(name, blobKey, fspp::Dir::EntryType::SYMLINK, S_IFLNK | S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IWGRP | S_IXGRP | S_IROTH | S_IWOTH | S_IXOTH, uid, gid, lastAccessTime, lastModificationTime); } @@ -89,41 +89,41 @@ void DirBlob::_addChild(const std::string &name, const Key &blobKey, void DirBlob::AddOrOverwriteChild(const std::string &name, const Key &blobKey, fspp::Dir::EntryType entryType, mode_t mode, uid_t uid, gid_t gid, timespec lastAccessTime, timespec lastModificationTime, std::function onOverwritten) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _entries.addOrOverwrite(name, blobKey, entryType, mode, uid, gid, lastAccessTime, lastModificationTime, onOverwritten); _changed = true; } void DirBlob::RenameChild(const blockstore::Key &key, const std::string &newName, std::function onOverwritten) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _entries.rename(key, 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 Key &key) const { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); return _entries.get(key); } 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 Key &key) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _entries.remove(key); _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()); @@ -135,7 +135,17 @@ off_t DirBlob::lstat_size() const { } void DirBlob::statChild(const Key &key, struct ::stat *result) const { - result->st_size = _getLstatSize(key); + 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(); + + result->st_size = lstatSizeGetter(key); statChildWithSizeAlreadySet(key, result); } @@ -159,43 +169,43 @@ void DirBlob::statChildWithSizeAlreadySet(const Key &key, struct ::stat *result) } void DirBlob::updateAccessTimestampForChild(const Key &key) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _entries.updateAccessTimestampForChild(key); _changed = true; } void DirBlob::updateModificationTimestampForChild(const Key &key) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _entries.updateModificationTimestampForChild(key); _changed = true; } void DirBlob::chmodChild(const Key &key, mode_t mode) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _entries.setMode(key, mode); _changed = true; } void DirBlob::chownChild(const Key &key, uid_t uid, gid_t gid) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); if(_entries.setUidGid(key, uid, gid)) { _changed = true; } } void DirBlob::utimensChild(const Key &key, timespec lastAccessTime, timespec lastModificationTime) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_entriesAndChangedMutex); _entries.setAccessTimes(key, lastAccessTime, lastModificationTime); _changed = true; } void DirBlob::setLstatSizeGetter(std::function getLstatSize) { - std::unique_lock lock(_mutex); + std::unique_lock lock(_getLstatSizeMutex); _getLstatSize = 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 4b4e71af..3202e41e 100644 --- a/src/cryfs/filesystem/fsblobstore/DirBlob.h +++ b/src/cryfs/filesystem/fsblobstore/DirBlob.h @@ -82,8 +82,9 @@ namespace cryfs { FsBlobStore *_fsBlobStore; 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 eed580a09088a6bc33df236a46e2278e708b44c8 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Thu, 27 Dec 2018 10:36:56 +0100 Subject: [PATCH 02/10] Fix compiler warning --- src/cpp-utils/random/ThreadsafeRandomDataBuffer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cpp-utils/random/ThreadsafeRandomDataBuffer.h b/src/cpp-utils/random/ThreadsafeRandomDataBuffer.h index a1047aa7..c9ee6938 100644 --- a/src/cpp-utils/random/ThreadsafeRandomDataBuffer.h +++ b/src/cpp-utils/random/ThreadsafeRandomDataBuffer.h @@ -54,7 +54,7 @@ namespace cpputils { inline size_t ThreadsafeRandomDataBuffer::_get(void *target, size_t numBytes) { boost::unique_lock lock(_mutex); - _dataAddedCv.wait(lock, [this, numBytes] { + _dataAddedCv.wait(lock, [this] { return _buffer.size() > 0; }); size_t gettableBytes = std::min(_buffer.size(), numBytes); From 72d9f29ef7edf103e7180ce9012ca413be700afb Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Thu, 27 Dec 2018 10:41:57 +0100 Subject: [PATCH 03/10] Update changelog --- ChangeLog.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ChangeLog.txt b/ChangeLog.txt index a7b85fe0..50c5bf2b 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -1,3 +1,8 @@ +Version 0.9.10 (unreleased) +-------------- +Fixed bugs: +* Fixed occasional deadlock (https://github.com/cryfs/cryfs/issues/64) + Version 0.9.9 -------------- Improvements: From d7ba25f55682d4ff3f15b3a732f9810a6bb2f278 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 12 Jan 2019 16:31:25 -0800 Subject: [PATCH 04/10] Fix Travis CI --- .travis.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.travis.yml b/.travis.yml index 7c919e56..b04ca620 100644 --- a/.travis.yml +++ b/.travis.yml @@ -18,6 +18,8 @@ addons: - libcrypto++-dev - libfuse-dev install: +# Workaround homebrew bug, see https://twitter.com/axccl/status/1083393735277363205 and https://github.com/openPMD/openPMD-api/pull/431/files +- if [ "${TRAVIS_OS_NAME}" == "osx" ]; then travis_wait brew upgrade --cleanup; travis_wait brew upgrade --cleanup; fi # Use new clang - if [ "${TRAVIS_OS_NAME}" == "linux" ] && [ "$CXX" = "clang++" ]; then export CXX="clang++-3.7" CC="clang-3.7"; fi # Detect number of CPU cores From 49a8c684a08229eae1e863b4bccb7052e0e32d57 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 12 Jan 2019 23:19:27 -0800 Subject: [PATCH 05/10] - Fix for reading empty files out of bounds - Fixed race condition (https://github.com/cryfs/cryfs/issues/224 and https://github.com/cryfs/cryfs/issues/243) --- ChangeLog.txt | 2 + .../implementations/onblocks/BlobOnBlocks.cpp | 56 ++++++++++-- .../implementations/onblocks/BlobOnBlocks.h | 5 +- .../onblocks/datatreestore/DataTree.cpp | 4 + .../onblocks/datatreestore/DataTree.h | 7 ++ .../parallelaccessdatatreestore/DataTreeRef.h | 6 ++ .../onblocks/BlobReadWriteTest.cpp | 86 +++++++++++++++++++ 7 files changed, 156 insertions(+), 10 deletions(-) diff --git a/ChangeLog.txt b/ChangeLog.txt index 50c5bf2b..a743ccf3 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -2,6 +2,8 @@ Version 0.9.10 (unreleased) -------------- Fixed bugs: * Fixed occasional deadlock (https://github.com/cryfs/cryfs/issues/64) +* Fix for reading empty files out of bounds +* Fixed race condition (https://github.com/cryfs/cryfs/issues/224) Version 0.9.9 -------------- diff --git a/src/blobstore/implementations/onblocks/BlobOnBlocks.cpp b/src/blobstore/implementations/onblocks/BlobOnBlocks.cpp index 174183ca..a228f2b4 100644 --- a/src/blobstore/implementations/onblocks/BlobOnBlocks.cpp +++ b/src/blobstore/implementations/onblocks/BlobOnBlocks.cpp @@ -26,6 +26,11 @@ BlobOnBlocks::~BlobOnBlocks() { } uint64_t BlobOnBlocks::size() const { + std::unique_lock lock(_datatree->mutex()); + return _size(); +} + +uint64_t BlobOnBlocks::_size() const { if (_sizeCache == boost::none) { _sizeCache = _datatree->numStoredBytes(); } @@ -33,15 +38,17 @@ uint64_t BlobOnBlocks::size() const { } void BlobOnBlocks::resize(uint64_t numBytes) { + std::unique_lock lock(_datatree->mutex()); + _datatree->resizeNumBytes(numBytes); _sizeCache = numBytes; } -void BlobOnBlocks::traverseLeaves(uint64_t beginByte, uint64_t sizeBytes, function func) const { +void BlobOnBlocks::_traverseLeaves(uint64_t beginByte, uint64_t sizeBytes, function func) const { uint64_t endByte = beginByte + sizeBytes; uint32_t firstLeaf = beginByte / _datatree->maxBytesPerLeaf(); uint32_t endLeaf = utils::ceilDivision(endByte, _datatree->maxBytesPerLeaf()); - bool writingOutside = size() < endByte; // TODO Calling size() is slow because it has to traverse the tree + bool writingOutside = _size() < endByte; // TODO Calling size() is slow because it has to traverse the tree _datatree->traverseLeaves(firstLeaf, endLeaf, [&func, beginByte, endByte, endLeaf, writingOutside](DataLeafNode *leaf, uint32_t leafIndex) { uint64_t indexOfFirstLeafByte = leafIndex * leaf->maxStoreableBytes(); uint32_t dataBegin = utils::maxZeroSubtraction(beginByte, indexOfFirstLeafByte); @@ -59,41 +66,70 @@ void BlobOnBlocks::traverseLeaves(uint64_t beginByte, uint64_t sizeBytes, functi } Data BlobOnBlocks::readAll() const { + std::unique_lock lock(_datatree->mutex()); + //TODO Querying size is inefficient. Is this possible without a call to size()? - uint64_t count = size(); + uint64_t count = _size(); Data result(count); _read(result.data(), 0, count); return result; } void BlobOnBlocks::read(void *target, uint64_t offset, uint64_t count) const { - ASSERT(offset <= size() && offset + count <= size(), "BlobOnBlocks::read() read outside blob. Use BlobOnBlocks::tryRead() if this should be allowed."); - uint64_t read = tryRead(target, offset, count); - ASSERT(read == count, "BlobOnBlocks::read() couldn't read all requested bytes. Use BlobOnBlocks::tryRead() if this should be allowed."); + std::unique_lock lock(_datatree->mutex()); + + if(offset > _size() || offset + count > _size()) { + throw std::runtime_error("BlobOnBlocks::read() read outside blob. Use BlobOnBlocks::tryRead() if this should be allowed."); + } + uint64_t read = _tryRead(target, offset, count); + if(read != count) { + throw std::runtime_error("BlobOnBlocks::read() couldn't read all requested bytes. Use BlobOnBlocks::tryRead() if this should be allowed."); + } } uint64_t BlobOnBlocks::tryRead(void *target, uint64_t offset, uint64_t count) const { + std::unique_lock lock(_datatree->mutex()); + return _tryRead(target, offset, count); +} + +uint64_t BlobOnBlocks::_tryRead(void *target, uint64_t offset, uint64_t count) const { + if (_size() <= offset) { + return 0; + } + //TODO Quite inefficient to call size() here, because that has to traverse the tree - uint64_t realCount = std::max(UINT64_C(0), std::min(count, size()-offset)); + uint64_t realCount = std::max(INT64_C(0), std::min(static_cast(count), static_cast(_size())-static_cast(offset))); _read(target, offset, realCount); return realCount; } void BlobOnBlocks::_read(void *target, uint64_t offset, uint64_t count) const { - traverseLeaves(offset, count, [target, offset] (uint64_t indexOfFirstLeafByte, const DataLeafNode *leaf, uint32_t leafDataOffset, uint32_t leafDataSize) { + if (count == 0) { + return; + } + + _traverseLeaves(offset, count, [target, offset] (uint64_t indexOfFirstLeafByte, const DataLeafNode *leaf, uint32_t leafDataOffset, uint32_t leafDataSize) { //TODO Simplify formula, make it easier to understand leaf->read((uint8_t*)target + indexOfFirstLeafByte - offset + leafDataOffset, leafDataOffset, leafDataSize); }); } void BlobOnBlocks::write(const void *source, uint64_t offset, uint64_t count) { - traverseLeaves(offset, count, [source, offset] (uint64_t indexOfFirstLeafByte, DataLeafNode *leaf, uint32_t leafDataOffset, uint32_t leafDataSize) { + if (count == 0) { + return; + } + + std::unique_lock lock(_datatree->mutex()); + + _traverseLeaves(offset, count, [source, offset] (uint64_t indexOfFirstLeafByte, DataLeafNode *leaf, uint32_t leafDataOffset, uint32_t leafDataSize) { //TODO Simplify formula, make it easier to understand leaf->write((uint8_t*)source + indexOfFirstLeafByte - offset + leafDataOffset, leafDataOffset, leafDataSize); }); } void BlobOnBlocks::flush() { + std::unique_lock lock(_datatree->mutex()); + _datatree->flush(); } @@ -102,6 +138,8 @@ const Key &BlobOnBlocks::key() const { } unique_ref BlobOnBlocks::releaseTree() { + std::unique_lock lock(_datatree->mutex()); + return std::move(_datatree); } diff --git a/src/blobstore/implementations/onblocks/BlobOnBlocks.h b/src/blobstore/implementations/onblocks/BlobOnBlocks.h index 7d73db39..aba0c983 100644 --- a/src/blobstore/implementations/onblocks/BlobOnBlocks.h +++ b/src/blobstore/implementations/onblocks/BlobOnBlocks.h @@ -37,8 +37,11 @@ public: private: + uint64_t _size() const; + void _read(void *target, uint64_t offset, uint64_t count) const; - void traverseLeaves(uint64_t offsetBytes, uint64_t sizeBytes, std::function) const; + uint64_t _tryRead(void *target, uint64_t offset, uint64_t size) const; + void _traverseLeaves(uint64_t offsetBytes, uint64_t sizeBytes, std::function) const; cpputils::unique_ref _datatree; mutable boost::optional _sizeCache; diff --git a/src/blobstore/implementations/onblocks/datatreestore/DataTree.cpp b/src/blobstore/implementations/onblocks/datatreestore/DataTree.cpp index 75209e25..e961472f 100644 --- a/src/blobstore/implementations/onblocks/datatreestore/DataTree.cpp +++ b/src/blobstore/implementations/onblocks/datatreestore/DataTree.cpp @@ -158,6 +158,10 @@ uint32_t DataTree::_numLeaves(const DataNode &node) const { } void DataTree::traverseLeaves(uint32_t beginIndex, uint32_t endIndex, function func) { + if (endIndex <= beginIndex) { + return; + } + //TODO Can we traverse in parallel? unique_lock lock(_mutex); //TODO Only lock when resizing. Otherwise parallel read/write to a blob is not possible! ASSERT(beginIndex <= endIndex, "Invalid parameters"); diff --git a/src/blobstore/implementations/onblocks/datatreestore/DataTree.h b/src/blobstore/implementations/onblocks/datatreestore/DataTree.h index ae7e6c43..4c9491a1 100644 --- a/src/blobstore/implementations/onblocks/datatreestore/DataTree.h +++ b/src/blobstore/implementations/onblocks/datatreestore/DataTree.h @@ -38,7 +38,14 @@ public: void flush() const; + // This is a hack to fix a race condition. This is only done in the 0.9 release branch to workaround the issue, + // the develop branch and 0.10 release series have a proper fix. + std::mutex& mutex() const { + return _outerMutex; + } + private: + mutable std::mutex _outerMutex; mutable boost::shared_mutex _mutex; datanodestore::DataNodeStore *_nodeStore; cpputils::unique_ref _rootNode; diff --git a/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/DataTreeRef.h b/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/DataTreeRef.h index b379c91a..ca2da24d 100644 --- a/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/DataTreeRef.h +++ b/src/blobstore/implementations/onblocks/parallelaccessdatatreestore/DataTreeRef.h @@ -41,6 +41,12 @@ public: return _baseTree->flush(); } + // This is a hack to fix a race condition. This is only done in the 0.9 release branch to workaround the issue, + // the develop branch and 0.10 release series have a proper fix. + std::mutex& mutex() const { + return _baseTree->mutex(); + } + private: datatreestore::DataTree *_baseTree; diff --git a/test/blobstore/implementations/onblocks/BlobReadWriteTest.cpp b/test/blobstore/implementations/onblocks/BlobReadWriteTest.cpp index e6459acb..45a4ea35 100644 --- a/test/blobstore/implementations/onblocks/BlobReadWriteTest.cpp +++ b/test/blobstore/implementations/onblocks/BlobReadWriteTest.cpp @@ -63,6 +63,92 @@ TEST_F(BlobReadWriteTest, WritingCloseTo16ByteLimitDoesntDestroySize) { EXPECT_EQ(32780u, blob->size()); } +TEST_F(BlobReadWriteTest, givenEmptyBlob_whenTryReadInFirstLeaf_thenFails) { + Data data(5); + size_t read = blob->tryRead(data.data(), 3, 5); + EXPECT_EQ(0, read); +} + +TEST_F(BlobReadWriteTest, givenEmptyBlob_whenTryReadInLaterLeaf_thenFails) { + Data data(5); + size_t read = blob->tryRead(data.data(), 2*LAYOUT.maxBytesPerLeaf(), 5); + EXPECT_EQ(0, read); +} + +TEST_F(BlobReadWriteTest, givenEmptyBlob_whenReadInFirstLeaf_thenFails) { + Data data(5); + EXPECT_ANY_THROW( + blob->read(data.data(), 3, 5) + ); +} + +TEST_F(BlobReadWriteTest, givenEmptyBlob_whenReadInLaterLeaf_thenFails) { + Data data(5); + EXPECT_ANY_THROW( + blob->read(data.data(), 2*LAYOUT.maxBytesPerLeaf(), 5) + ); +} + +TEST_F(BlobReadWriteTest, givenEmptyBlob_whenReadAll_thenReturnsZeroSizedData) { + Data data = blob->readAll(); + EXPECT_EQ(0, data.size()); +} + +TEST_F(BlobReadWriteTest, givenEmptyBlob_whenWrite_thenGrows) { + Data data(5); + blob->write(data.data(), 4, 5); + EXPECT_EQ(9, blob->size()); +} + +TEST_F(BlobReadWriteTest, givenEmptyBlob_whenWriteZeroBytes_thenDoesntGrow) { + Data data(5); + blob->write(data.data(), 4, 0); + EXPECT_EQ(0, blob->size());; +} + +TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenTryReadInFirstLeaf_thenFails) { + Data data(5); + size_t read = blob->tryRead(data.data(), 3, 5); + EXPECT_EQ(0, read); +} + +TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenTryReadInLaterLeaf_thenFails) { + Data data(5); + size_t read = blob->tryRead(data.data(), 2*LAYOUT.maxBytesPerLeaf(), 5); + EXPECT_EQ(0, read); +} + +TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenReadInFirstLeaf_thenFails) { + Data data(5); + EXPECT_ANY_THROW( + blob->read(data.data(), 3, 5) + ); +} + +TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenReadInLaterLeaf_thenFails) { + Data data(5); + EXPECT_ANY_THROW( + blob->read(data.data(), 2*LAYOUT.maxBytesPerLeaf(), 5) + ); +} + +TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenReadAll_thenReturnsZeroSizedData) { + Data data = blob->readAll(); + EXPECT_EQ(0, data.size()); +} + +TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenWrite_thenGrows) { + Data data(5); + blob->write(data.data(), 4, 5); + EXPECT_EQ(9, blob->size()); +} + +TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenWriteZeroBytes_thenDoesntGrow) { + Data data(5); + blob->write(data.data(), 4, 0); + EXPECT_EQ(0, blob->size()); +} + struct DataRange { size_t blobsize; off_t offset; From 5163368c81cdb1a2a53b6cf144743b7ea13c29c0 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 12 Jan 2019 23:26:38 -0800 Subject: [PATCH 06/10] Changelog --- ChangeLog.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.txt b/ChangeLog.txt index a743ccf3..39ad077c 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -3,7 +3,7 @@ Version 0.9.10 (unreleased) Fixed bugs: * Fixed occasional deadlock (https://github.com/cryfs/cryfs/issues/64) * Fix for reading empty files out of bounds -* Fixed race condition (https://github.com/cryfs/cryfs/issues/224) +* Fixed race condition (https://github.com/cryfs/cryfs/issues/224 and https://github.com/cryfs/cryfs/issues/243) Version 0.9.9 -------------- From 86600e725345b7864be2ca3c394d682996dc0814 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sun, 20 Jan 2019 19:39:01 -0800 Subject: [PATCH 07/10] Mark 0.9.10 as released --- ChangeLog.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.txt b/ChangeLog.txt index 39ad077c..7a599f89 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -1,4 +1,4 @@ -Version 0.9.10 (unreleased) +Version 0.9.10 -------------- Fixed bugs: * Fixed occasional deadlock (https://github.com/cryfs/cryfs/issues/64) From e90fda0f03b7b819d4de844bd07a9c1e60454c99 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 1 Jun 2019 20:01:35 -0700 Subject: [PATCH 08/10] Fix a race condition when a file descriptor is closed while there's read/write requests for that file being processed. --- ChangeLog.txt | 7 ++- src/fspp/impl/FilesystemImpl.cpp | 28 ++++++--- src/fspp/impl/FuseOpenFileList.h | 78 +++++++++++++++++++++++-- src/fspp/impl/IdList.h | 19 +++--- test/fspp/impl/FuseOpenFileListTest.cpp | 22 +++---- 5 files changed, 123 insertions(+), 31 deletions(-) diff --git a/ChangeLog.txt b/ChangeLog.txt index 39ad077c..2dde72e0 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -1,4 +1,9 @@ -Version 0.9.10 (unreleased) +Version 0.9.11 (unreleased) +-------------- +Fixed bugs: +* Fix a race condition when a file descriptor is closed while there's read/write requests for that file being processed. + +Version 0.9.10 -------------- Fixed bugs: * Fixed occasional deadlock (https://github.com/cryfs/cryfs/issues/64) diff --git a/src/fspp/impl/FilesystemImpl.cpp b/src/fspp/impl/FilesystemImpl.cpp index 14de38ec..398292e3 100644 --- a/src/fspp/impl/FilesystemImpl.cpp +++ b/src/fspp/impl/FilesystemImpl.cpp @@ -131,7 +131,9 @@ int FilesystemImpl::openFile(File *file, int flags) { void FilesystemImpl::flush(int descriptor) { PROFILE(_flushNanosec); - _open_files.get(descriptor)->flush(); + _open_files.load(descriptor, [](OpenFile* openFile) { + openFile->flush(); + }); } void FilesystemImpl::closeFile(int descriptor) { @@ -151,7 +153,9 @@ void FilesystemImpl::lstat(const bf::path &path, struct ::stat *stbuf) { void FilesystemImpl::fstat(int descriptor, struct ::stat *stbuf) { PROFILE(_fstatNanosec); - _open_files.get(descriptor)->stat(stbuf); + _open_files.load(descriptor, [&](OpenFile* openFile) { + openFile->stat(stbuf); + }); } void FilesystemImpl::chmod(const boost::filesystem::path &path, mode_t mode) { @@ -181,27 +185,37 @@ void FilesystemImpl::truncate(const bf::path &path, off_t size) { void FilesystemImpl::ftruncate(int descriptor, off_t size) { PROFILE(_ftruncateNanosec); - _open_files.get(descriptor)->truncate(size); + _open_files.load(descriptor, [size] (OpenFile* openFile) { + openFile->truncate(size); + }); } size_t FilesystemImpl::read(int descriptor, void *buf, size_t count, off_t offset) { PROFILE(_readNanosec); - return _open_files.get(descriptor)->read(buf, count, offset); + return _open_files.load(descriptor, [buf, count, offset] (OpenFile* openFile) { + return openFile->read(buf, count, offset); + }); } void FilesystemImpl::write(int descriptor, const void *buf, size_t count, off_t offset) { PROFILE(_writeNanosec); - _open_files.get(descriptor)->write(buf, count, offset); + return _open_files.load(descriptor, [buf, count, offset] (OpenFile* openFile) { + return openFile->write(buf, count, offset); + }); } void FilesystemImpl::fsync(int descriptor) { PROFILE(_fsyncNanosec); - _open_files.get(descriptor)->fsync(); + _open_files.load(descriptor, [] (OpenFile* openFile) { + openFile->fsync(); + }); } void FilesystemImpl::fdatasync(int descriptor) { PROFILE(_fdatasyncNanosec); - _open_files.get(descriptor)->fdatasync(); + _open_files.load(descriptor, [] (OpenFile* openFile) { + openFile->fdatasync(); + }); } void FilesystemImpl::access(const bf::path &path, int mask) { diff --git a/src/fspp/impl/FuseOpenFileList.h b/src/fspp/impl/FuseOpenFileList.h index ec1d1d18..795a6fb5 100644 --- a/src/fspp/impl/FuseOpenFileList.h +++ b/src/fspp/impl/FuseOpenFileList.h @@ -4,10 +4,27 @@ #include "../fs_interface/File.h" #include "../fs_interface/OpenFile.h" +#include "../fs_interface/FuseErrnoException.h" #include +#include #include "IdList.h" +#include namespace fspp { +namespace detail { +class OnScopeExit final { +public: + explicit OnScopeExit(std::function handler) + : _handler(std::move(handler)) {} + + ~OnScopeExit() { + _handler(); + } + +private: + std::function _handler; +}; +} class FuseOpenFileList final { public: @@ -15,33 +32,84 @@ public: ~FuseOpenFileList(); int open(cpputils::unique_ref file); - OpenFile *get(int descriptor); + template + auto load(int descriptor, Func&& callback); void close(int descriptor); private: IdList _open_files; + std::unordered_map _refcounts; + std::mutex _mutex; + + std::condition_variable _refcount_zero_cv; + DISALLOW_COPY_AND_ASSIGN(FuseOpenFileList); }; inline FuseOpenFileList::FuseOpenFileList() - :_open_files() { + :_open_files(), _refcounts(), _mutex(), _refcount_zero_cv() { } inline FuseOpenFileList::~FuseOpenFileList() { + std::unique_lock lock(_mutex); + + // Wait until all pending requests are done + _refcount_zero_cv.wait(lock, [&] { + for (const auto& refcount : _refcounts) { + if (0 != refcount.second) { + return false; + } + } + return true; + }); + + // There might still be open files when the file system is shutdown, so we can't assert it's empty. + // But to check that _refcounts has been updated correctly, we can assert the invariant that we have as many + // refcounts as open files. + ASSERT(_refcounts.size() == _refcounts.size(), "Didn't clean up refcounts properly"); } inline int FuseOpenFileList::open(cpputils::unique_ref file) { - return _open_files.add(std::move(file)); + std::lock_guard lock(_mutex); + + int descriptor = _open_files.add(std::move(file)); + _refcounts.emplace(descriptor, 0); + return descriptor; } -inline OpenFile *FuseOpenFileList::get(int descriptor) { - return _open_files.get(descriptor); +template +inline auto FuseOpenFileList::load(int descriptor, Func&& callback) { + try { + std::unique_lock lock(_mutex); + _refcounts.at(descriptor) += 1; + detail::OnScopeExit _([&] { + if (!lock.owns_lock()) { // own_lock can be true when _open_files.get() below fails before the lock is unlocked + lock.lock(); + } + _refcounts.at(descriptor) -= 1; + _refcount_zero_cv.notify_all(); + }); + + OpenFile* loaded = _open_files.get(descriptor); + lock.unlock(); + + return std::forward(callback)(loaded); + } catch (const std::out_of_range& e) { + throw fspp::fuse::FuseErrnoException(EBADF); + } } inline void FuseOpenFileList::close(int descriptor) { + std::unique_lock lock(_mutex); + + _refcount_zero_cv.wait(lock, [&] { + return 0 == _refcounts.at(descriptor); + }); + //The destructor of the stored FuseOpenFile closes the file _open_files.remove(descriptor); + _refcounts.erase(descriptor); } } diff --git a/src/fspp/impl/IdList.h b/src/fspp/impl/IdList.h index 41c9308e..c0b9a16b 100644 --- a/src/fspp/impl/IdList.h +++ b/src/fspp/impl/IdList.h @@ -2,7 +2,7 @@ #ifndef MESSMER_FSPP_IMPL_IDLIST_H_ #define MESSMER_FSPP_IMPL_IDLIST_H_ -#include +#include #include #include #include @@ -19,17 +19,18 @@ public: Entry *get(int id); const Entry *get(int id) const; void remove(int id); + size_t size() const; + private: - std::map> _entries; + std::unordered_map> _entries; int _id_counter; - mutable std::mutex _mutex; DISALLOW_COPY_AND_ASSIGN(IdList); }; template IdList::IdList() - : _entries(), _id_counter(0), _mutex() { + : _entries(), _id_counter(0) { } template @@ -38,10 +39,9 @@ IdList::~IdList() { template int IdList::add(cpputils::unique_ref entry) { - std::lock_guard lock(_mutex); //TODO Reuse IDs (ids = descriptors) int new_id = ++_id_counter; - _entries.insert(std::make_pair(new_id, std::move(entry))); + _entries.emplace(new_id, std::move(entry)); return new_id; } @@ -52,14 +52,12 @@ Entry *IdList::get(int id) { template const Entry *IdList::get(int id) const { - std::lock_guard lock(_mutex); const Entry *result = _entries.at(id).get(); return result; } template void IdList::remove(int id) { - std::lock_guard lock(_mutex); auto found_iter = _entries.find(id); if (found_iter == _entries.end()) { throw std::out_of_range("Called IdList::remove() with an invalid ID"); @@ -67,6 +65,11 @@ void IdList::remove(int id) { _entries.erase(found_iter); } +template +size_t IdList::size() const { + return _entries.size(); +} + } #endif diff --git a/test/fspp/impl/FuseOpenFileListTest.cpp b/test/fspp/impl/FuseOpenFileListTest.cpp index be104fd8..bfef6e26 100644 --- a/test/fspp/impl/FuseOpenFileListTest.cpp +++ b/test/fspp/impl/FuseOpenFileListTest.cpp @@ -45,24 +45,26 @@ struct FuseOpenFileListTest: public ::testing::Test { return open(FILEID1, FILEID2); } void check(int id, int fileid, int flags) { - MockOpenFile *openFile = dynamic_cast(list.get(id)); - EXPECT_EQ(fileid, openFile->fileid); - EXPECT_EQ(flags, openFile->flags); + list.load(id, [=](OpenFile* _openFile) { + MockOpenFile *openFile = dynamic_cast(_openFile); + EXPECT_EQ(fileid, openFile->fileid); + EXPECT_EQ(flags, openFile->flags); + }); } }; TEST_F(FuseOpenFileListTest, EmptyList1) { - ASSERT_THROW(list.get(0), std::out_of_range); + ASSERT_THROW(list.load(0, [](OpenFile*) {}), fspp::fuse::FuseErrnoException); } TEST_F(FuseOpenFileListTest, EmptyList2) { - ASSERT_THROW(list.get(3), std::out_of_range); + ASSERT_THROW(list.load(3, [](OpenFile*) {}), fspp::fuse::FuseErrnoException); } TEST_F(FuseOpenFileListTest, InvalidId) { int valid_id = open(); int invalid_id = valid_id + 1; - ASSERT_THROW(list.get(invalid_id), std::out_of_range); + ASSERT_THROW(list.load(invalid_id, [](OpenFile*) {}), fspp::fuse::FuseErrnoException); } TEST_F(FuseOpenFileListTest, Open1AndGet) { @@ -102,18 +104,18 @@ TEST_F(FuseOpenFileListTest, Open3AndGet) { TEST_F(FuseOpenFileListTest, GetClosedItemOnEmptyList) { int id = open(); - ASSERT_NO_THROW(list.get(id)); + ASSERT_NO_THROW(list.load(id, [](OpenFile*) {})); list.close(id); - ASSERT_THROW(list.get(id), std::out_of_range); + ASSERT_THROW(list.load(id, [](OpenFile*) {}), fspp::fuse::FuseErrnoException); } TEST_F(FuseOpenFileListTest, GetClosedItemOnNonEmptyList) { int id = open(); open(); - ASSERT_NO_THROW(list.get(id)); + ASSERT_NO_THROW(list.load(id, [](OpenFile*) {})); list.close(id); - ASSERT_THROW(list.get(id), std::out_of_range); + ASSERT_THROW(list.load(id, [](OpenFile*) {}), fspp::fuse::FuseErrnoException); } TEST_F(FuseOpenFileListTest, CloseOnEmptyList1) { From 95d16471528254087ecdf39c07dee14cf0d180f1 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 1 Jun 2019 21:15:46 -0700 Subject: [PATCH 09/10] Fix --- src/fspp/impl/FuseOpenFileList.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fspp/impl/FuseOpenFileList.h b/src/fspp/impl/FuseOpenFileList.h index 795a6fb5..d87ccbfc 100644 --- a/src/fspp/impl/FuseOpenFileList.h +++ b/src/fspp/impl/FuseOpenFileList.h @@ -4,7 +4,7 @@ #include "../fs_interface/File.h" #include "../fs_interface/OpenFile.h" -#include "../fs_interface/FuseErrnoException.h" +#include "../fuse/FuseErrnoException.h" #include #include #include "IdList.h" From 19f6296023496ad551fa1c9005c23fbe5e306673 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sun, 2 Jun 2019 09:18:28 -0700 Subject: [PATCH 10/10] Fix Travis CI --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index b04ca620..9a9d3702 100644 --- a/.travis.yml +++ b/.travis.yml @@ -19,7 +19,7 @@ addons: - libfuse-dev install: # Workaround homebrew bug, see https://twitter.com/axccl/status/1083393735277363205 and https://github.com/openPMD/openPMD-api/pull/431/files -- if [ "${TRAVIS_OS_NAME}" == "osx" ]; then travis_wait brew upgrade --cleanup; travis_wait brew upgrade --cleanup; fi +- if [ "${TRAVIS_OS_NAME}" == "osx" ]; then travis_wait brew upgrade; travis_wait brew upgrade; fi # Use new clang - if [ "${TRAVIS_OS_NAME}" == "linux" ] && [ "$CXX" = "clang++" ]; then export CXX="clang++-3.7" CC="clang-3.7"; fi # Detect number of CPU cores