From 752be4415ce1a0c40606b09a6fa04386ad4f1493 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Wed, 30 Sep 2015 14:05:05 +0200 Subject: [PATCH] Protect from race conditions happening when the same FsBlob is open multiple times --- src/filesystem/fsblobstore/DirBlob.cpp | 8 +++--- src/filesystem/fsblobstore/DirBlob.h | 5 ++-- src/filesystem/fsblobstore/FileBlob.cpp | 8 +++--- src/filesystem/fsblobstore/FileBlob.h | 4 +-- src/filesystem/fsblobstore/FsBlob.h | 8 ++++-- src/filesystem/fsblobstore/FsBlobStore.cpp | 30 +++++++++++++++++----- src/filesystem/fsblobstore/FsBlobStore.h | 7 +++++ src/filesystem/fsblobstore/SymlinkBlob.cpp | 8 +++--- src/filesystem/fsblobstore/SymlinkBlob.h | 5 ++-- src/main.cpp | 1 + 10 files changed, 58 insertions(+), 26 deletions(-) diff --git a/src/filesystem/fsblobstore/DirBlob.cpp b/src/filesystem/fsblobstore/DirBlob.cpp index 26192cee..609e4ca2 100644 --- a/src/filesystem/fsblobstore/DirBlob.cpp +++ b/src/filesystem/fsblobstore/DirBlob.cpp @@ -28,8 +28,8 @@ namespace fsblobstore { //TODO Factor out a DirEntryList class -DirBlob::DirBlob(unique_ref blob, FsBlobStore *fsBlobStore) : - FsBlob(std::move(blob)), _fsBlobStore(fsBlobStore), _entries(), _changed(false) { +DirBlob::DirBlob(unique_ref blob, FsBlobStore *fsBlobStore, std::function onDestruct) : + FsBlob(std::move(blob), onDestruct), _fsBlobStore(fsBlobStore), _entries(), _changed(false) { ASSERT(magicNumber() == MagicNumbers::DIR, "Loaded blob is not a directory"); _readEntriesFromBlob(); } @@ -43,9 +43,9 @@ void DirBlob::flush() { baseBlob().flush(); } -unique_ref DirBlob::InitializeEmptyDir(unique_ref blob, FsBlobStore *fsBlobStore) { +unique_ref DirBlob::InitializeEmptyDir(unique_ref blob, FsBlobStore *fsBlobStore, std::function onDestruct) { InitializeBlobWithMagicNumber(blob.get(), MagicNumbers::DIR); - return make_unique_ref(std::move(blob), fsBlobStore); + return make_unique_ref(std::move(blob), fsBlobStore, onDestruct); } void DirBlob::_writeEntriesToBlob() { diff --git a/src/filesystem/fsblobstore/DirBlob.h b/src/filesystem/fsblobstore/DirBlob.h index 3603be67..686ac1ff 100644 --- a/src/filesystem/fsblobstore/DirBlob.h +++ b/src/filesystem/fsblobstore/DirBlob.h @@ -43,9 +43,10 @@ namespace cryfs { }; static cpputils::unique_ref InitializeEmptyDir(cpputils::unique_ref blob, - FsBlobStore *fsBlobStore); + FsBlobStore *fsBlobStore, + std::function onDestruct); - DirBlob(cpputils::unique_ref blob, FsBlobStore *fsBlobStore); + DirBlob(cpputils::unique_ref blob, FsBlobStore *fsBlobStore, std::function onDestruct); virtual ~DirBlob(); diff --git a/src/filesystem/fsblobstore/FileBlob.cpp b/src/filesystem/fsblobstore/FileBlob.cpp index dafce824..479bae33 100644 --- a/src/filesystem/fsblobstore/FileBlob.cpp +++ b/src/filesystem/fsblobstore/FileBlob.cpp @@ -11,13 +11,13 @@ using cpputils::make_unique_ref; namespace cryfs { namespace fsblobstore { -FileBlob::FileBlob(unique_ref blob) -: FsBlob(std::move(blob)) { +FileBlob::FileBlob(unique_ref blob, std::function onDestruct) +: FsBlob(std::move(blob), onDestruct) { } -unique_ref FileBlob::InitializeEmptyFile(unique_ref blob) { +unique_ref FileBlob::InitializeEmptyFile(unique_ref blob, std::function onDestruct) { InitializeBlobWithMagicNumber(blob.get(), MagicNumbers::FILE); - return make_unique_ref(std::move(blob)); + return make_unique_ref(std::move(blob), onDestruct); } ssize_t FileBlob::read(void *target, uint64_t offset, uint64_t count) const { diff --git a/src/filesystem/fsblobstore/FileBlob.h b/src/filesystem/fsblobstore/FileBlob.h index c540c79b..0886f887 100644 --- a/src/filesystem/fsblobstore/FileBlob.h +++ b/src/filesystem/fsblobstore/FileBlob.h @@ -9,9 +9,9 @@ namespace cryfs { class FileBlob: public FsBlob { public: - static cpputils::unique_ref InitializeEmptyFile(cpputils::unique_ref blob); + static cpputils::unique_ref InitializeEmptyFile(cpputils::unique_ref blob, std::function onDestruct); - FileBlob(cpputils::unique_ref blob); + FileBlob(cpputils::unique_ref blob, std::function onDestruct); ssize_t read(void *target, uint64_t offset, uint64_t count) const; diff --git a/src/filesystem/fsblobstore/FsBlob.h b/src/filesystem/fsblobstore/FsBlob.h index 96ab3850..8196700d 100644 --- a/src/filesystem/fsblobstore/FsBlob.h +++ b/src/filesystem/fsblobstore/FsBlob.h @@ -3,6 +3,7 @@ #include #include +#include namespace cryfs { namespace fsblobstore { @@ -14,7 +15,7 @@ namespace cryfs { blockstore::Key key() const; protected: - FsBlob(cpputils::unique_ref baseBlob); + FsBlob(cpputils::unique_ref baseBlob, std::function onDestruct); blobstore::Blob &baseBlob(); const blobstore::Blob &baseBlob() const; @@ -29,14 +30,17 @@ namespace cryfs { cpputils::unique_ref releaseBaseBlob(); cpputils::unique_ref _baseBlob; + std::function _onDestruct; DISALLOW_COPY_AND_ASSIGN(FsBlob); }; - inline FsBlob::FsBlob(cpputils::unique_ref baseBlob): _baseBlob(std::move(baseBlob)) { + inline FsBlob::FsBlob(cpputils::unique_ref baseBlob, std::function onDestruct) + : _baseBlob(std::move(baseBlob)), _onDestruct(onDestruct) { } inline FsBlob::~FsBlob() { + _onDestruct(); } inline blockstore::Key FsBlob::key() const { diff --git a/src/filesystem/fsblobstore/FsBlobStore.cpp b/src/filesystem/fsblobstore/FsBlobStore.cpp index 8cceb717..05a5e305 100644 --- a/src/filesystem/fsblobstore/FsBlobStore.cpp +++ b/src/filesystem/fsblobstore/FsBlobStore.cpp @@ -9,6 +9,7 @@ using cpputils::unique_ref; using cpputils::make_unique_ref; using blobstore::BlobStore; using boost::none; +using std::function; namespace cryfs { namespace fsblobstore { @@ -17,31 +18,42 @@ FsBlobStore::FsBlobStore(unique_ref baseBlobStore): _baseBlobStore(st } unique_ref FsBlobStore::createFileBlob() { - return FileBlob::InitializeEmptyFile(_baseBlobStore->create()); + auto blob = _baseBlobStore->create(); + auto key = blob->key(); + _openBlobs.lock(key); + return FileBlob::InitializeEmptyFile(std::move(blob), freeLockFunction(key)); } unique_ref FsBlobStore::createDirBlob() { + auto blob = _baseBlobStore->create(); + auto key = blob->key(); + _openBlobs.lock(key); //TODO Passed in fsBlobStore should be ParallelAccessFsBlobStore later - return DirBlob::InitializeEmptyDir(_baseBlobStore->create(), this); + return DirBlob::InitializeEmptyDir(std::move(blob), this, freeLockFunction(key)); } unique_ref FsBlobStore::createSymlinkBlob(const bf::path &target) { - return SymlinkBlob::InitializeSymlink(_baseBlobStore->create(), target); + auto blob = _baseBlobStore->create(); + auto key = blob->key(); + _openBlobs.lock(key); + return SymlinkBlob::InitializeSymlink(std::move(blob), target, freeLockFunction(key)); } boost::optional> FsBlobStore::load(const blockstore::Key &key) { + _openBlobs.lock(key); + auto blob = _baseBlobStore->load(key); if (blob == none) { return none; } unsigned char magicNumber = FsBlob::magicNumber(**blob); if (magicNumber == MagicNumbers::FILE) { - return unique_ref(make_unique_ref(std::move(*blob))); + return unique_ref(make_unique_ref(std::move(*blob), freeLockFunction(key))); } else if (magicNumber == MagicNumbers::DIR) { //TODO Passed in fsBlobStore should be ParallelAccessFsBlobStore later - return unique_ref(make_unique_ref(std::move(*blob), this)); + return unique_ref(make_unique_ref(std::move(*blob), this, freeLockFunction(key))); } else if (magicNumber == MagicNumbers::SYMLINK) { - return unique_ref(make_unique_ref(std::move(*blob))); + return unique_ref(make_unique_ref(std::move(*blob), freeLockFunction(key))); } else { ASSERT(false, "Unknown magic number"); } @@ -51,5 +63,11 @@ void FsBlobStore::remove(cpputils::unique_ref blob) { _baseBlobStore->remove(blob->releaseBaseBlob()); } +function FsBlobStore::freeLockFunction(const blockstore::Key &key) { + return [this, key] { + _openBlobs.release(key); + }; +} + } } \ No newline at end of file diff --git a/src/filesystem/fsblobstore/FsBlobStore.h b/src/filesystem/fsblobstore/FsBlobStore.h index a71f591f..4565efd1 100644 --- a/src/filesystem/fsblobstore/FsBlobStore.h +++ b/src/filesystem/fsblobstore/FsBlobStore.h @@ -1,6 +1,7 @@ #ifndef CRYFS_FSBLOBSTORE_FSBLOBSTORE_H #define CRYFS_FSBLOBSTORE_FSBLOBSTORE_H +#include #include #include #include "FileBlob.h" @@ -9,6 +10,8 @@ namespace cryfs { namespace fsblobstore { + //TODO Test classes in fsblobstore + class FsBlobStore { public: FsBlobStore(cpputils::unique_ref baseBlobStore); @@ -20,6 +23,10 @@ namespace cryfs { void remove(cpputils::unique_ref blob); private: + std::function freeLockFunction(const blockstore::Key &key); + + //Instead of locking open blobs, it would be faster to allow parallel access similar to parallelaccessstore. + cpputils::LockPool _openBlobs; cpputils::unique_ref _baseBlobStore; }; } diff --git a/src/filesystem/fsblobstore/SymlinkBlob.cpp b/src/filesystem/fsblobstore/SymlinkBlob.cpp index 44298da3..c8d59451 100644 --- a/src/filesystem/fsblobstore/SymlinkBlob.cpp +++ b/src/filesystem/fsblobstore/SymlinkBlob.cpp @@ -14,17 +14,17 @@ namespace bf = boost::filesystem; namespace cryfs { namespace fsblobstore { -SymlinkBlob::SymlinkBlob(unique_ref blob) -: FsBlob(std::move(blob)), _target(_readTargetFromBlob(baseBlob())) { +SymlinkBlob::SymlinkBlob(unique_ref blob, std::function onDestruct) +: FsBlob(std::move(blob), onDestruct), _target(_readTargetFromBlob(baseBlob())) { } -unique_ref SymlinkBlob::InitializeSymlink(unique_ref blob, const bf::path &target) { +unique_ref SymlinkBlob::InitializeSymlink(unique_ref blob, const bf::path &target, std::function onDestruct) { string targetStr = target.native(); blob->resize(1 + targetStr.size()); unsigned char magicNumber = MagicNumbers::SYMLINK; blob->write(&magicNumber, 0, 1); blob->write(targetStr.c_str(), 1, targetStr.size()); - return make_unique_ref(std::move(blob)); + return make_unique_ref(std::move(blob), onDestruct); } void SymlinkBlob::_checkMagicNumber(const Blob &blob) { diff --git a/src/filesystem/fsblobstore/SymlinkBlob.h b/src/filesystem/fsblobstore/SymlinkBlob.h index 07f51989..c9371e10 100644 --- a/src/filesystem/fsblobstore/SymlinkBlob.h +++ b/src/filesystem/fsblobstore/SymlinkBlob.h @@ -11,9 +11,10 @@ namespace cryfs { class SymlinkBlob: public FsBlob { public: static cpputils::unique_ref InitializeSymlink(cpputils::unique_ref blob, - const boost::filesystem::path &target); + const boost::filesystem::path &target, + std::function onDestruct); - SymlinkBlob(cpputils::unique_ref blob); + SymlinkBlob(cpputils::unique_ref blob, std::function onDestruct); const boost::filesystem::path &target() const; diff --git a/src/main.cpp b/src/main.cpp index af72fda7..fd3ddabf 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -27,6 +27,7 @@ using std::endl; using std::vector; //TODO Support files > 4GB +//TODO cryfs process doesn't seem to react to "kill". Needs "kill -9". Why? Furthermore, calling "fusermount -u" unmounts the fs, but the cryfs process keeps running. Why? void showVersion() { cout << "CryFS Version " << version::VERSION_STRING << endl;