From e90fda0f03b7b819d4de844bd07a9c1e60454c99 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 1 Jun 2019 20:01:35 -0700 Subject: [PATCH] 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) {