From 0cdfb8ba2e667afd8baeb60db6d309023bc87126 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Wed, 16 Mar 2016 16:57:46 +0000 Subject: [PATCH] Fix fstat (a bug in the fstat implementation caused problems with some text editors (e.g. nano) falsely thinking a file changed since they opened it). --- ChangeLog.txt | 1 + src/cryfs/filesystem/CryDir.cpp | 5 ++-- src/cryfs/filesystem/CryFile.cpp | 2 +- src/cryfs/filesystem/CryNode.cpp | 23 +++++++++++++------ src/cryfs/filesystem/CryNode.h | 3 ++- src/cryfs/filesystem/CryOpenFile.cpp | 9 ++++---- src/cryfs/filesystem/CryOpenFile.h | 4 +++- .../cachingfsblobstore/DirBlobRef.h | 4 ++++ src/cryfs/filesystem/fsblobstore/DirBlob.cpp | 8 ++++--- src/cryfs/filesystem/fsblobstore/DirBlob.h | 2 ++ .../parallelaccessfsblobstore/DirBlobRef.h | 4 ++++ src/fspp/fstest/testutils/FileTest.h | 3 +-- 12 files changed, 47 insertions(+), 21 deletions(-) diff --git a/ChangeLog.txt b/ChangeLog.txt index c6a11ae8..4eec9d85 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -6,6 +6,7 @@ Version 0.9.3 (unreleased) * You can disable the automatic update check by setting CRYFS_NO_UPDATE_CHECK=true in your environment. * Building CryFS from the GitHub tarball (i.e. when there is no .git directory present) works. * The ciphertext block size is configurable. You can use the "--blocksize-bytes" command line argument. If not specified, CryFS will ask you for a block size when creating a file system. +* Fix fstat (a bug in the fstat implementation caused problems with some text editors (e.g. nano) falsely thinking a file changed since they opened it). Version 0.9.2 --------------- diff --git a/src/cryfs/filesystem/CryDir.cpp b/src/cryfs/filesystem/CryDir.cpp index 1ce6db2e..cb0bb859 100644 --- a/src/cryfs/filesystem/CryDir.cpp +++ b/src/cryfs/filesystem/CryDir.cpp @@ -41,8 +41,9 @@ unique_ref CryDir::createAndOpenFile(const string &name, mode_t device()->callFsActionCallbacks(); auto child = device()->CreateFileBlob(); auto now = fsblobstore::time::now(); - LoadBlob()->AddChildFile(name, child->key(), mode, uid, gid, now, now); - return make_unique_ref(device(), std::move(child)); + auto dirBlob = LoadBlob(); + dirBlob->AddChildFile(name, child->key(), mode, uid, gid, now, now); + return make_unique_ref(device(), cpputils::to_unique_ptr(std::move(dirBlob)), std::move(child)); } void CryDir::createDir(const string &name, mode_t mode, uid_t uid, gid_t gid) { diff --git a/src/cryfs/filesystem/CryFile.cpp b/src/cryfs/filesystem/CryFile.cpp index cdde1c2d..cedc64e4 100644 --- a/src/cryfs/filesystem/CryFile.cpp +++ b/src/cryfs/filesystem/CryFile.cpp @@ -37,7 +37,7 @@ unique_ref CryFile::LoadBlob() const { unique_ref CryFile::open(int flags) const { device()->callFsActionCallbacks(); auto blob = LoadBlob(); - return make_unique_ref(device(), std::move(blob)); + return make_unique_ref(device(), parent(), std::move(blob)); } void CryFile::truncate(off_t size) const { diff --git a/src/cryfs/filesystem/CryNode.cpp b/src/cryfs/filesystem/CryNode.cpp index 86885a23..98115864 100644 --- a/src/cryfs/filesystem/CryNode.cpp +++ b/src/cryfs/filesystem/CryNode.cpp @@ -17,6 +17,7 @@ using cpputils::dynamic_pointer_move; using cpputils::unique_ref; using boost::optional; using boost::none; +using std::shared_ptr; using cryfs::parallelaccessfsblobstore::FsBlobRef; using cryfs::parallelaccessfsblobstore::DirBlobRef; @@ -28,8 +29,11 @@ namespace cryfs { CryNode::CryNode(CryDevice *device, optional> parent, const Key &key) : _device(device), - _parent(std::move(parent)), + _parent(none), _key(key) { + if (parent != none) { + _parent = cpputils::to_unique_ptr(std::move(*parent)); + } } CryNode::~CryNode() { @@ -42,10 +46,15 @@ void CryNode::access(int mask) const { throw FuseErrnoException(ENOTSUP); } +shared_ptr CryNode::parent() const { + ASSERT(_parent != none, "We are the root directory and can't get the parent of the root directory"); + return *_parent; +} + void CryNode::rename(const bf::path &to) { device()->callFsActionCallbacks(); if (_parent == none) { - //We are the base direcory. + //We are the root direcory. //TODO What should we do? throw FuseErrnoException(EIO); } @@ -70,7 +79,7 @@ void CryNode::rename(const bf::path &to) { void CryNode::utimens(timespec lastAccessTime, timespec lastModificationTime) { device()->callFsActionCallbacks(); if (_parent == none) { - //We are the base direcory. + //We are the root direcory. //TODO What should we do? throw FuseErrnoException(EIO); } @@ -80,7 +89,7 @@ void CryNode::utimens(timespec lastAccessTime, timespec lastModificationTime) { void CryNode::removeNode() { //TODO Instead of all these if-else and having _parent being an optional, we could also introduce a CryRootDir which inherits from fspp::Dir. if (_parent == none) { - //We are the base direcory. + //We are the root direcory. //TODO What should we do? throw FuseErrnoException(EIO); } @@ -103,7 +112,7 @@ unique_ref CryNode::LoadBlob() const { void CryNode::stat(struct ::stat *result) const { device()->callFsActionCallbacks(); if(_parent == none) { - //We are the base directory. + //We are the root directory. //TODO What should we do? result->st_uid = getuid(); result->st_gid = getgid(); @@ -130,7 +139,7 @@ void CryNode::stat(struct ::stat *result) const { void CryNode::chmod(mode_t mode) { device()->callFsActionCallbacks(); if (_parent == none) { - //We are the base direcory. + //We are the root direcory. //TODO What should we do? throw FuseErrnoException(EIO); } @@ -140,7 +149,7 @@ void CryNode::chmod(mode_t mode) { void CryNode::chown(uid_t uid, gid_t gid) { device()->callFsActionCallbacks(); if (_parent == none) { - //We are the base direcory. + //We are the root direcory. //TODO What should we do? throw FuseErrnoException(EIO); } diff --git a/src/cryfs/filesystem/CryNode.h b/src/cryfs/filesystem/CryNode.h index 4639f24c..fbb811d1 100644 --- a/src/cryfs/filesystem/CryNode.h +++ b/src/cryfs/filesystem/CryNode.h @@ -27,6 +27,7 @@ protected: CryDevice *device(); const CryDevice *device() const; cpputils::unique_ref LoadBlob() const; + std::shared_ptr parent() const; virtual fspp::Dir::EntryType getType() const = 0; @@ -34,7 +35,7 @@ protected: private: CryDevice *_device; - boost::optional> _parent; + boost::optional> _parent; blockstore::Key _key; DISALLOW_COPY_AND_ASSIGN(CryNode); diff --git a/src/cryfs/filesystem/CryOpenFile.cpp b/src/cryfs/filesystem/CryOpenFile.cpp index 70585874..62aad73d 100644 --- a/src/cryfs/filesystem/CryOpenFile.cpp +++ b/src/cryfs/filesystem/CryOpenFile.cpp @@ -8,8 +8,10 @@ namespace bf = boost::filesystem; +using std::shared_ptr; using cpputils::unique_ref; using cryfs::parallelaccessfsblobstore::FileBlobRef; +using cryfs::parallelaccessfsblobstore::DirBlobRef; //TODO Get rid of this in favor of a exception hierarchy using fspp::fuse::CHECK_RETVAL; @@ -17,8 +19,8 @@ using fspp::fuse::FuseErrnoException; namespace cryfs { -CryOpenFile::CryOpenFile(const CryDevice *device, unique_ref fileBlob) -: _device(device), _fileBlob(std::move(fileBlob)) { +CryOpenFile::CryOpenFile(const CryDevice *device, shared_ptr parent, unique_ref fileBlob) +: _device(device), _parent(parent), _fileBlob(std::move(fileBlob)) { } CryOpenFile::~CryOpenFile() { @@ -32,9 +34,8 @@ void CryOpenFile::flush() { void CryOpenFile::stat(struct ::stat *result) const { _device->callFsActionCallbacks(); - result->st_mode = S_IFREG | S_IRUSR | S_IXUSR | S_IWUSR; + _parent->statChildExceptSize(_fileBlob->key(), result); result->st_size = _fileBlob->size(); - return; } void CryOpenFile::truncate(off_t size) const { diff --git a/src/cryfs/filesystem/CryOpenFile.h b/src/cryfs/filesystem/CryOpenFile.h index 7fdc906a..b621e3be 100644 --- a/src/cryfs/filesystem/CryOpenFile.h +++ b/src/cryfs/filesystem/CryOpenFile.h @@ -4,13 +4,14 @@ #include #include "parallelaccessfsblobstore/FileBlobRef.h" +#include "parallelaccessfsblobstore/DirBlobRef.h" namespace cryfs { class CryDevice; class CryOpenFile final: public fspp::OpenFile { public: - explicit CryOpenFile(const CryDevice *device, cpputils::unique_ref fileBlob); + explicit CryOpenFile(const CryDevice *device, std::shared_ptr parent, cpputils::unique_ref fileBlob); ~CryOpenFile(); void stat(struct ::stat *result) const override; @@ -23,6 +24,7 @@ public: private: const CryDevice *_device; + std::shared_ptr _parent; cpputils::unique_ref _fileBlob; DISALLOW_COPY_AND_ASSIGN(CryOpenFile); diff --git a/src/cryfs/filesystem/cachingfsblobstore/DirBlobRef.h b/src/cryfs/filesystem/cachingfsblobstore/DirBlobRef.h index 640289c2..7c5eed72 100644 --- a/src/cryfs/filesystem/cachingfsblobstore/DirBlobRef.h +++ b/src/cryfs/filesystem/cachingfsblobstore/DirBlobRef.h @@ -47,6 +47,10 @@ public: return _base->statChild(key, result); } + void statChildExceptSize(const blockstore::Key &key, struct ::stat *result) const { + return _base->statChildExceptSize(key, result); + } + void chmodChild(const blockstore::Key &key, mode_t mode) { return _base->chmodChild(key, mode); } diff --git a/src/cryfs/filesystem/fsblobstore/DirBlob.cpp b/src/cryfs/filesystem/fsblobstore/DirBlob.cpp index 4b4cae47..9b4d188c 100644 --- a/src/cryfs/filesystem/fsblobstore/DirBlob.cpp +++ b/src/cryfs/filesystem/fsblobstore/DirBlob.cpp @@ -112,13 +112,16 @@ off_t DirBlob::lstat_size() const { } void DirBlob::statChild(const Key &key, struct ::stat *result) const { + statChildExceptSize(key, result); + result->st_size = _getLstatSize(key); +} + +void DirBlob::statChildExceptSize(const Key &key, struct ::stat *result) const { auto childOpt = GetChild(key); if (childOpt == boost::none) { throw fspp::fuse::FuseErrnoException(ENOENT); } const auto &child = *childOpt; - //TODO Loading the blob for only getting the size of the file/symlink is not very performant. - // Furthermore, this is the only reason why DirBlob needs a pointer to _fsBlobStore, which is ugly result->st_mode = child.mode(); result->st_uid = child.uid(); result->st_gid = child.gid(); @@ -133,7 +136,6 @@ void DirBlob::statChild(const Key &key, struct ::stat *result) const { result->st_mtim = child.lastModificationTime(); result->st_ctim = child.lastMetadataChangeTime(); #endif - result->st_size = _getLstatSize(key); //TODO Move ceilDivision to general utils which can be used by cryfs as well result->st_blocks = blobstore::onblocks::utils::ceilDivision(result->st_size, (off_t)512); result->st_blksize = _fsBlobStore->blocksizeBytes(); diff --git a/src/cryfs/filesystem/fsblobstore/DirBlob.h b/src/cryfs/filesystem/fsblobstore/DirBlob.h index 48669219..316c00a9 100644 --- a/src/cryfs/filesystem/fsblobstore/DirBlob.h +++ b/src/cryfs/filesystem/fsblobstore/DirBlob.h @@ -52,6 +52,8 @@ namespace cryfs { void statChild(const blockstore::Key &key, struct ::stat *result) const; + void statChildExceptSize(const blockstore::Key &key, struct ::stat *result) const; + void chmodChild(const blockstore::Key &key, mode_t mode); void chownChild(const blockstore::Key &key, uid_t uid, gid_t gid); diff --git a/src/cryfs/filesystem/parallelaccessfsblobstore/DirBlobRef.h b/src/cryfs/filesystem/parallelaccessfsblobstore/DirBlobRef.h index 87ce75c1..113b9be8 100644 --- a/src/cryfs/filesystem/parallelaccessfsblobstore/DirBlobRef.h +++ b/src/cryfs/filesystem/parallelaccessfsblobstore/DirBlobRef.h @@ -43,6 +43,10 @@ public: return _base->statChild(key, result); } + void statChildExceptSize(const blockstore::Key &key, struct ::stat *result) const { + return _base->statChildExceptSize(key, result); + } + void chmodChild(const blockstore::Key &key, mode_t mode) { return _base->chmodChild(key, mode); } diff --git a/src/fspp/fstest/testutils/FileTest.h b/src/fspp/fstest/testutils/FileTest.h index 64ff94df..b8d6c337 100644 --- a/src/fspp/fstest/testutils/FileTest.h +++ b/src/fspp/fstest/testutils/FileTest.h @@ -25,8 +25,7 @@ public: file.stat(&st1); callback(st1); file.open(O_RDONLY)->stat(&st2); - //TODO Enable this line - //callback(st2); + callback(st2); } void EXPECT_SIZE(uint64_t expectedSize, const fspp::File &file) {