From 3e15eadec930b4b049f0feed032425b35163293c Mon Sep 17 00:00:00 2001 From: Valient Gough Date: Fri, 4 Aug 2017 23:35:43 -0700 Subject: [PATCH] rewrite mempool using thread-local storage, update cmake feature checks --- CMakeLists.txt | 2 + config.h.cmake | 4 ++ encfs/BlockFileIO.cpp | 56 +++++++----------- encfs/FileNode.cpp | 4 +- encfs/MACFileIO.cpp | 17 ++---- encfs/MemoryPool.cpp | 135 ++++++++++++++++++++++-------------------- encfs/MemoryPool.h | 52 ++++++++++------ encfs/RawFileIO.cpp | 10 +++- encfs/SSL_Cipher.cpp | 52 ++++++++-------- encfs/main.cpp | 2 +- encfs/test.cpp | 34 +++++------ 11 files changed, 188 insertions(+), 180 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4829e25..b71257a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -86,6 +86,7 @@ find_program (POD2MAN pod2man) include (CheckIncludeFileCXX) check_include_file_cxx (attr/xattr.h HAVE_ATTR_XATTR_H) check_include_file_cxx (sys/xattr.h HAVE_SYS_XATTR_H) +check_include_file_cxx (valgrind/memcheck.h HAVE_VALGRIND_MEMCHECK_H) # Check if xattr functions take extra arguments, as they do on OSX. include (CheckCXXSourceCompiles) @@ -98,6 +99,7 @@ check_cxx_source_compiles ("#include include (CheckFuncs) check_function_exists_glibc (lchmod HAVE_LCHMOD) check_function_exists_glibc (utimensat HAVE_UTIMENSAT) +check_function_exists_glibc (fdatasync HAVE_FDATASYNC) set (CMAKE_THREAD_PREFER_PTHREAD) find_package (Threads REQUIRED) diff --git a/config.h.cmake b/config.h.cmake index 1e2fdc4..73b66a1 100644 --- a/config.h.cmake +++ b/config.h.cmake @@ -2,10 +2,14 @@ #cmakedefine HAVE_ATTR_XATTR_H #cmakedefine HAVE_SYS_XATTR_H +#cmakedefine HAVE_VALGRIND_MEMCHECK_H + #cmakedefine XATTR_ADD_OPT #cmakedefine XATTR_LLIST #cmakedefine HAVE_LCHMOD +#cmakedefine HAVE_UTIMENSAT +#cmakedefine HAVE_FDATASYNC /* TODO: add other thread library support. */ #cmakedefine CMAKE_USE_PTHREADS_INIT diff --git a/encfs/BlockFileIO.cpp b/encfs/BlockFileIO.cpp index ee9a835..d8e40b1 100644 --- a/encfs/BlockFileIO.cpp +++ b/encfs/BlockFileIO.cpp @@ -133,11 +133,12 @@ ssize_t BlockFileIO::read(const IORequest &req) const { // if the request is larger then a block, then request each block // individually - MemBlock mb; // in case we need to allocate a temporary block.. + MemBlock mb{}; // in case we need to allocate a temporary block.. IORequest blockReq; // for requests we may need to make blockReq.dataLen = _blockSize; blockReq.data = nullptr; + rAssert(req.data != nullptr); unsigned char *out = req.data; while (size != 0u) { blockReq.offset = blockNum * _blockSize; @@ -147,10 +148,8 @@ ssize_t BlockFileIO::read(const IORequest &req) const { if (partialOffset == 0 && size >= (size_t)_blockSize) { blockReq.data = out; } else { - if (mb.data == nullptr) { - mb = MemoryPool::allocate(_blockSize); - } - blockReq.data = mb.data; + mb.allocate(_blockSize); + blockReq.data = mb.get(); } ssize_t readSize = cacheReadOneBlock(blockReq); @@ -163,6 +162,7 @@ ssize_t BlockFileIO::read(const IORequest &req) const { // if we read to a temporary buffer, then move the data if (blockReq.data != out) { + rAssert(blockReq.data != nullptr); memcpy(out, blockReq.data + partialOffset, cpySize); } @@ -177,10 +177,6 @@ ssize_t BlockFileIO::read(const IORequest &req) const { } } - if (mb.data != nullptr) { - MemoryPool::release(mb); - } - return result; } @@ -227,7 +223,7 @@ bool BlockFileIO::write(const IORequest &req) { } // have to merge data with existing block(s).. - MemBlock mb; + MemBlock mb{}; IORequest blockReq; blockReq.data = nullptr; @@ -250,11 +246,9 @@ bool BlockFileIO::write(const IORequest &req) { } else { // need a temporary buffer, since we have to either merge or pad // the data. - if (mb.data == nullptr) { - mb = MemoryPool::allocate(_blockSize); - } - memset(mb.data, 0, _blockSize); - blockReq.data = mb.data; + mb.allocate(_blockSize); + blockReq.data = mb.get(); + memset(blockReq.data, 0, _blockSize); if (blockNum > lastNonEmptyBlock) { // just pad.. @@ -286,10 +280,6 @@ bool BlockFileIO::write(const IORequest &req) { partialOffset = 0; } - if (mb.data != nullptr) { - MemoryPool::release(mb); - } - return ok; } @@ -301,22 +291,22 @@ void BlockFileIO::padFile(off_t oldSize, off_t newSize, bool forceWrite) { int newBlockSize = newSize % _blockSize; IORequest req; - MemBlock mb; + MemBlock mb{}; if (oldLastBlock == newLastBlock) { // when the real write occurs, it will have to read in the existing // data and pad it anyway, so we won't do it here (unless we're // forced). if (forceWrite) { - mb = MemoryPool::allocate(_blockSize); - req.data = mb.data; + mb.allocate(_blockSize); + req.data = mb.get(); req.offset = oldLastBlock * _blockSize; req.dataLen = oldSize % _blockSize; int outSize = newSize % _blockSize; // outSize > req.dataLen if (outSize != 0) { - memset(mb.data, 0, outSize); + memset(mb.get(), 0, outSize); cacheReadOneBlock(req); req.dataLen = outSize; cacheWriteOneBlock(req); @@ -324,8 +314,8 @@ void BlockFileIO::padFile(off_t oldSize, off_t newSize, bool forceWrite) { } else VLOG(1) << "optimization: not padding last block"; } else { - mb = MemoryPool::allocate(_blockSize); - req.data = mb.data; + mb.allocate(_blockSize); + req.data = mb.get(); // 1. extend the first block to full length // 2. write the middle empty blocks @@ -337,7 +327,7 @@ void BlockFileIO::padFile(off_t oldSize, off_t newSize, bool forceWrite) { // 1. req.dataLen == 0, iff oldSize was already a multiple of blocksize if (req.dataLen != 0) { VLOG(1) << "padding block " << oldLastBlock; - memset(mb.data, 0, _blockSize); + memset(mb.get(), 0, _blockSize); cacheReadOneBlock(req); req.dataLen = _blockSize; // expand to full block size cacheWriteOneBlock(req); @@ -350,7 +340,7 @@ void BlockFileIO::padFile(off_t oldSize, off_t newSize, bool forceWrite) { VLOG(1) << "padding block " << oldLastBlock; req.offset = oldLastBlock * _blockSize; req.dataLen = _blockSize; - memset(mb.data, 0, req.dataLen); + memset(mb.get(), 0, req.dataLen); cacheWriteOneBlock(req); } } @@ -359,14 +349,10 @@ void BlockFileIO::padFile(off_t oldSize, off_t newSize, bool forceWrite) { if (forceWrite && (newBlockSize != 0)) { req.offset = newLastBlock * _blockSize; req.dataLen = newBlockSize; - memset(mb.data, 0, req.dataLen); + memset(mb.get(), 0, req.dataLen); cacheWriteOneBlock(req); } } - - if (mb.data != nullptr) { - MemoryPool::release(mb); - } } int BlockFileIO::truncateBase(off_t size, FileIO *base) { @@ -393,12 +379,12 @@ int BlockFileIO::truncateBase(off_t size, FileIO *base) { // truncated before the truncate. Then write it back out afterwards, // since the encoding will change.. off_t blockNum = size / _blockSize; - MemBlock mb = MemoryPool::allocate(_blockSize); + MemBlock mb(_blockSize); IORequest req; req.offset = blockNum * _blockSize; req.dataLen = _blockSize; - req.data = mb.data; + req.data = mb.get(); ssize_t rdSz = cacheReadOneBlock(req); @@ -416,8 +402,6 @@ int BlockFileIO::truncateBase(off_t size, FileIO *base) { RLOG(WARNING) << "truncate failure: read " << rdSz << " bytes, partial block of " << partialBlock; } - - MemoryPool::release(mb); } else { // truncating on a block bounday. No need to re-encode the last // block.. diff --git a/encfs/FileNode.cpp b/encfs/FileNode.cpp index 4ae65a7..d03180b 100644 --- a/encfs/FileNode.cpp +++ b/encfs/FileNode.cpp @@ -30,6 +30,7 @@ #include +#include "config.h" #include "CipherFileIO.h" #include "Error.h" #include "FileIO.h" @@ -258,7 +259,7 @@ int FileNode::sync(bool datasync) { int fh = io->open(O_RDONLY); if (fh >= 0) { int res = -EIO; -#ifdef linux +#if defined(HAVE_FDATASYNC) if (datasync) { res = fdatasync(fh); } else { @@ -267,7 +268,6 @@ int FileNode::sync(bool datasync) { #else (void)datasync; // no fdatasync support - // TODO: use autoconfig to check for it.. res = fsync(fh); #endif diff --git a/encfs/MACFileIO.cpp b/encfs/MACFileIO.cpp index bc03605..590cc17 100644 --- a/encfs/MACFileIO.cpp +++ b/encfs/MACFileIO.cpp @@ -149,11 +149,11 @@ ssize_t MACFileIO::readOneBlock(const IORequest &req) const { int bs = blockSize() + headerSize; - MemBlock mb = MemoryPool::allocate(bs); + MemBlock mb(bs); IORequest tmp; tmp.offset = locWithHeader(req.offset, bs, headerSize); - tmp.data = mb.data; + tmp.data = mb.get(); tmp.dataLen = headerSize + req.dataLen; // get the data from the base FileIO layer @@ -193,7 +193,6 @@ ssize_t MACFileIO::readOneBlock(const IORequest &req) const { long blockNum = req.offset / bs; RLOG(WARNING) << "MAC comparison failure in block " << blockNum; if (!warnOnly) { - MemoryPool::release(mb); throw Error(_("MAC comparison failure, refusing to read")); } } @@ -209,8 +208,6 @@ ssize_t MACFileIO::readOneBlock(const IORequest &req) const { } } - MemoryPool::release(mb); - return readSize; } @@ -220,11 +217,11 @@ bool MACFileIO::writeOneBlock(const IORequest &req) { int bs = blockSize() + headerSize; // we have the unencrypted data, so we need to attach a header to it. - MemBlock mb = MemoryPool::allocate(bs); + MemBlock mb(bs); IORequest newReq; newReq.offset = locWithHeader(req.offset, bs, headerSize); - newReq.data = mb.data; + newReq.data = mb.get(); newReq.dataLen = headerSize + req.dataLen; memset(newReq.data, 0, headerSize); @@ -247,11 +244,7 @@ bool MACFileIO::writeOneBlock(const IORequest &req) { } // now, we can let the next level have it.. - bool ok = base->write(newReq); - - MemoryPool::release(mb); - - return ok; + return base->write(newReq); } int MACFileIO::truncate(off_t size) { diff --git a/encfs/MemoryPool.cpp b/encfs/MemoryPool.cpp index ff28056..270aaad 100644 --- a/encfs/MemoryPool.cpp +++ b/encfs/MemoryPool.cpp @@ -18,11 +18,11 @@ * along with this program. If not, see . */ +#include "config.h" #include "MemoryPool.h" #include #include -#include #ifdef HAVE_VALGRIND_MEMCHECK_H #include @@ -33,100 +33,109 @@ #include -#define BLOCKDATA(BLOCK) (unsigned char *)BLOCK->data->data - namespace encfs { +static thread_local BlockList *gMemPool = nullptr; + struct BlockList { BlockList *next; - int size; - BUF_MEM *data; + BUF_MEM *buf; + + unsigned char *get(); + int size(); + void clear(); + + explicit BlockList(int size); + ~BlockList(); + + BlockList(const BlockList &) = delete; + BlockList &operator=(const BlockList &) = delete; }; -static BlockList *allocBlock(int size) { - auto *block = new BlockList; - block->size = size; - block->data = BUF_MEM_new(); - BUF_MEM_grow(block->data, size); - VALGRIND_MAKE_MEM_NOACCESS(block->data->data, block->data->max); - - return block; +unsigned char *BlockList::get() { + return reinterpret_cast(buf->data); } -static void freeBlock(BlockList *el) { - VALGRIND_MAKE_MEM_UNDEFINED(el->data->data, el->data->max); - BUF_MEM_free(el->data); +int BlockList::size() { return buf->max; } - delete el; +void BlockList::clear() { + VALGRIND_MAKE_MEM_UNDEFINED(buf->data, buf->max); + memset(buf->data, 0, buf->max); + VALGRIND_MAKE_MEM_NOACCESS(buf->data, buf->max); } -static pthread_mutex_t gMPoolMutex = PTHREAD_MUTEX_INITIALIZER; -static BlockList *gMemPool = nullptr; +BlockList::BlockList(int sz) { + buf = BUF_MEM_new(); + BUF_MEM_grow(buf, sz); + VALGRIND_MAKE_MEM_NOACCESS(buf->data, buf->max); +} -MemBlock MemoryPool::allocate(int size) { - pthread_mutex_lock(&gMPoolMutex); +BlockList::~BlockList() { + VALGRIND_MAKE_MEM_UNDEFINED(buf->data, buf->max); + BUF_MEM_free(buf); +} +static BlockList *allocateBlock(int size) { + // check if we already have a large enough block available.. BlockList *parent = nullptr; BlockList *block = gMemPool; - // check if we already have a large enough block available.. - while (block != nullptr && block->size < size) { + while (block != nullptr && block->size() < size) { parent = block; block = block->next; } - // unlink block from list - if (block != nullptr) { - if (parent == nullptr) { - gMemPool = block->next; - } else { - parent->next = block->next; - } - } - pthread_mutex_unlock(&gMPoolMutex); - if (block == nullptr) { - block = allocBlock(size); + // Allocate a new block. + return new BlockList(size); + } + + // unlink block from list + if (parent == nullptr) { + gMemPool = block->next; + } else { + parent->next = block->next; } block->next = nullptr; - - MemBlock result; - result.data = BLOCKDATA(block); - result.internalData = block; - - VALGRIND_MAKE_MEM_UNDEFINED(result.data, size); - - return result; + VALGRIND_MAKE_MEM_UNDEFINED(block->get(), size); + return block; } -void MemoryPool::release(const MemBlock &mb) { - pthread_mutex_lock(&gMPoolMutex); +MemBlock::MemBlock(int size) : bl(nullptr) { allocate(size); } - auto *block = (BlockList *)mb.internalData; +MemBlock::~MemBlock() { + if (bl == nullptr) { + return; + } - // just to be sure there's nothing important left in buffers.. - VALGRIND_MAKE_MEM_UNDEFINED(block->data->data, block->size); - memset(BLOCKDATA(block), 0, block->size); - VALGRIND_MAKE_MEM_NOACCESS(block->data->data, block->data->max); - - block->next = gMemPool; - gMemPool = block; - - pthread_mutex_unlock(&gMPoolMutex); + bl->clear(); + bl->next = gMemPool; + gMemPool = bl; + bl = nullptr; + data = nullptr; } -void MemoryPool::destroyAll() { - pthread_mutex_lock(&gMPoolMutex); +void MemBlock::allocate(int size) { + if (bl != nullptr) { + if (size <= bl->size()) { + return; + } - BlockList *block = gMemPool; - gMemPool = nullptr; + // Return existing block. + bl->clear(); + bl->next = gMemPool; + gMemPool = bl; + } - pthread_mutex_unlock(&gMPoolMutex); + bl = allocateBlock(size); + data = bl->get(); +} - while (block != nullptr) { - BlockList *next = block->next; +void MemBlock::freeAll() { + while (gMemPool != nullptr) { + BlockList *next = gMemPool->next; - freeBlock(block); - block = next; + delete gMemPool; + gMemPool = next; } } diff --git a/encfs/MemoryPool.h b/encfs/MemoryPool.h index fda3f27..967c409 100644 --- a/encfs/MemoryPool.h +++ b/encfs/MemoryPool.h @@ -23,29 +23,43 @@ namespace encfs { -struct MemBlock { +struct BlockList; + +// MemBlock holds a memory block stored in secure memory +// (if possible with the crypto backend). Blocks should be +// of consistent size, as the allocator is meant for working with +// file crypt blocks. +// +// To get a block, construct a MemBlock and call allocate(), or +// use the constructor with size. This either grabs a block from +// the thread-local block queue, or else constructs a new block. +// +// When the MemBlock instance is destroyed, the block is returned +// to the list for reuse. +class MemBlock { +public: + MemBlock() =default; + explicit MemBlock(int size); + ~MemBlock(); + + bool valid() const; + void allocate(int size); + + unsigned char *get() const; + + static void freeAll(); + +private: unsigned char *data; - - void *internalData; - - MemBlock(); + BlockList *bl; }; -inline MemBlock::MemBlock() : data(0), internalData(0) {} +inline bool MemBlock::valid() const { + return bl != nullptr; +} -/* - Memory Pool for fixed sized objects. - - Usage: - MemBlock mb = MemoryPool::allocate( size ); - // do things with storage in mb.data - unsigned char *buffer = mb.data; - MemoryPool::release( mb ); -*/ -namespace MemoryPool { -MemBlock allocate(int size); -void release(const MemBlock &el); -void destroyAll(); +inline unsigned char *MemBlock::get() const { + return data; } } // namespace encfs diff --git a/encfs/RawFileIO.cpp b/encfs/RawFileIO.cpp index b8dae9a..4d36d7a 100644 --- a/encfs/RawFileIO.cpp +++ b/encfs/RawFileIO.cpp @@ -33,6 +33,7 @@ #include "Error.h" #include "FileIO.h" #include "RawFileIO.h" +#include "config.h" using namespace std; @@ -257,9 +258,6 @@ int RawFileIO::truncate(off_t size) { if (fd >= 0 && canWrite) { res = ::ftruncate(fd, size); -#if !defined(__FreeBSD__) && !defined(__APPLE__) - ::fdatasync(fd); -#endif } else { res = ::truncate(name.c_str(), size); } @@ -276,6 +274,12 @@ int RawFileIO::truncate(off_t size) { knownSize = true; } +#if defined(HAVE_FDATASYNC) + if (fd >= 0 && canWrite) { + ::fdatasync(fd); + } +#endif + return res; } diff --git a/encfs/SSL_Cipher.cpp b/encfs/SSL_Cipher.cpp index c68466b..e0566a3 100644 --- a/encfs/SSL_Cipher.cpp +++ b/encfs/SSL_Cipher.cpp @@ -161,48 +161,48 @@ int TimedPBKDF2(const char *pass, int passlen, const unsigned char *salt, // - Version 3:0 adds a new IV mechanism static Interface BlowfishInterface("ssl/blowfish", 3, 0, 2); static Interface AESInterface("ssl/aes", 3, 0, 2); -static Interface CAMELLIAInterface("ssl/camellia",3, 0, 2); +static Interface CAMELLIAInterface("ssl/camellia", 3, 0, 2); #ifndef OPENSSL_NO_CAMELLIA static Range CAMELLIAKeyRange(128, 256, 64); static Range CAMELLIABlockRange(64, 4096, 16); -static std::shared_ptr NewCAMELLIACipher(const Interface &iface, int keyLen) { - if (keyLen <= 0) keyLen = 192; +static std::shared_ptr NewCAMELLIACipher(const Interface &iface, + int keyLen) { + if (keyLen <= 0) keyLen = 192; - keyLen = CAMELLIAKeyRange.closest(keyLen); + keyLen = CAMELLIAKeyRange.closest(keyLen); - const EVP_CIPHER *blockCipher = 0; - const EVP_CIPHER *streamCipher = 0; + const EVP_CIPHER *blockCipher = 0; + const EVP_CIPHER *streamCipher = 0; - switch (keyLen) { - case 128: - blockCipher = EVP_camellia_128_cbc(); - streamCipher = EVP_camellia_128_cfb(); - break; - case 192: - blockCipher = EVP_camellia_192_cbc(); - streamCipher = EVP_camellia_192_cfb(); - break; - case 256: - default: - blockCipher = EVP_camellia_256_cbc(); - streamCipher = EVP_camellia_256_cfb(); - break; - } + switch (keyLen) { + case 128: + blockCipher = EVP_camellia_128_cbc(); + streamCipher = EVP_camellia_128_cfb(); + break; + case 192: + blockCipher = EVP_camellia_192_cbc(); + streamCipher = EVP_camellia_192_cfb(); + break; + case 256: + default: + blockCipher = EVP_camellia_256_cbc(); + streamCipher = EVP_camellia_256_cfb(); + break; + } - return std::shared_ptr(new SSL_Cipher( - iface, CAMELLIAInterface, blockCipher, streamCipher, keyLen / 8 )); + return std::shared_ptr(new SSL_Cipher( + iface, CAMELLIAInterface, blockCipher, streamCipher, keyLen / 8)); } static bool CAMELLIA_Cipher_registered = - Cipher::Register("CAMELLIA","16 byte block cipher", CAMELLIAInterface, CAMELLIAKeyRange, - CAMELLIABlockRange, NewCAMELLIACipher ); + Cipher::Register("CAMELLIA", "16 byte block cipher", CAMELLIAInterface, + CAMELLIAKeyRange, CAMELLIABlockRange, NewCAMELLIACipher); #endif - #ifndef OPENSSL_NO_BF static Range BFKeyRange(128, 256, 32); diff --git a/encfs/main.cpp b/encfs/main.cpp index 0d0e6bb..af9a006 100644 --- a/encfs/main.cpp +++ b/encfs/main.cpp @@ -724,7 +724,7 @@ int main(int argc, char *argv[]) { rootInfo.reset(); ctx->setRoot(std::shared_ptr()); - MemoryPool::destroyAll(); + MemBlock::freeAll(); openssl_shutdown(encfsArgs->isThreaded); return returnCode; diff --git a/encfs/test.cpp b/encfs/test.cpp index dff8bbf..c53f063 100644 --- a/encfs/test.cpp +++ b/encfs/test.cpp @@ -54,41 +54,39 @@ const int FSBlockSize = 256; static int checkErrorPropogation(const std::shared_ptr &cipher, int size, int byteToChange, const CipherKey &key) { - MemBlock orig = MemoryPool::allocate(size); - MemBlock data = MemoryPool::allocate(size); + MemBlock orig(size); + MemBlock data(size); for (int i = 0; i < size; ++i) { unsigned char tmp = rand(); - orig.data[i] = tmp; - data.data[i] = tmp; + orig.get()[i] = tmp; + data.get()[i] = tmp; } - if (size != FSBlockSize) - cipher->streamEncode(data.data, size, 0, key); - else - cipher->blockEncode(data.data, size, 0, key); + if (size != FSBlockSize) { + cipher->streamEncode(data.get(), size, 0, key); + } else { + cipher->blockEncode(data.get(), size, 0, key); + } // intoduce an error in the encoded data, so we can check error propogation if (byteToChange >= 0 && byteToChange < size) { - unsigned char previousValue = data.data[byteToChange]; + unsigned char previousValue = data.get()[byteToChange]; do { - data.data[byteToChange] = rand(); - } while (data.data[byteToChange] == previousValue); + data.get()[byteToChange] = rand(); + } while (data.get()[byteToChange] == previousValue); } if (size != FSBlockSize) - cipher->streamDecode(data.data, size, 0, key); + cipher->streamDecode(data.get(), size, 0, key); else - cipher->blockDecode(data.data, size, 0, key); + cipher->blockDecode(data.get(), size, 0, key); int numByteErrors = 0; for (int i = 0; i < size; ++i) { - if (data.data[i] != orig.data[i]) ++numByteErrors; + if (data.get()[i] != orig.get()[i]) ++numByteErrors; } - MemoryPool::release(data); - MemoryPool::release(orig); - return numByteErrors; } @@ -452,7 +450,7 @@ int main(int argc, char *argv[]) { runTests(cipher, true); } - MemoryPool::destroyAll(); + MemBlock::freeAll(); return 0; }