rewrite mempool using thread-local storage, update cmake feature checks

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 <sys/types.h>
 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)

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

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.allocate(_blockSize);
-        mb = MemoryPool::allocate(_blockSize);
+      blockReq.data = mb.get();
-      }
-      blockReq.data = mb.data;
     }
 
     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.allocate(_blockSize);
-        mb = MemoryPool::allocate(_blockSize);
+      blockReq.data = mb.get();
-      }
+      memset(blockReq.data, 0, _blockSize);
-      memset(mb.data, 0, _blockSize);
-      blockReq.data = mb.data;
 
       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);
+      mb.allocate(_blockSize);
-      req.data = mb.data;
+      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);
+    mb.allocate(_blockSize);
-    req.data = mb.data;
+    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..

encfs/FileNode.cpp

@@ -30,6 +30,7 @@
 
 #include <cstring>
 
+#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
 

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);
+  return base->write(newReq);
-
-  MemoryPool::release(mb);
-
-  return ok;
 }
 
 int MACFileIO::truncate(off_t size) {

encfs/MemoryPool.cpp

@@ -18,11 +18,11 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "config.h"
 #include "MemoryPool.h"
 
 #include <cstring>
 #include <openssl/ossl_typ.h>
-#include <pthread.h>
 
 #ifdef HAVE_VALGRIND_MEMCHECK_H
 #include <valgrind/memcheck.h>
@@ -33,100 +33,109 @@
 
 #include <openssl/buffer.h>
 
-#define BLOCKDATA(BLOCK) (unsigned char *)BLOCK->data->data
-
 namespace encfs {
 
+static thread_local BlockList *gMemPool = nullptr;
+
 struct BlockList {
   BlockList *next;
-  int size;
+  BUF_MEM *buf;
-  BUF_MEM *data;
+
+  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) {
+unsigned char *BlockList::get() {
-  auto *block = new BlockList;
+  return reinterpret_cast<unsigned char *>(buf->data);
-  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;
 }
 
-static void freeBlock(BlockList *el) {
+int BlockList::size() { return buf->max; }
-  VALGRIND_MAKE_MEM_UNDEFINED(el->data->data, el->data->max);
-  BUF_MEM_free(el->data);
 
-  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;
+BlockList::BlockList(int sz) {
-static BlockList *gMemPool = nullptr;
+  buf = BUF_MEM_new();
+  BUF_MEM_grow(buf, sz);
+  VALGRIND_MAKE_MEM_NOACCESS(buf->data, buf->max);
+}
 
-MemBlock MemoryPool::allocate(int size) {
+BlockList::~BlockList() {
-  pthread_mutex_lock(&gMPoolMutex);
+  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;
   }
 
+  if (block == nullptr) {
+    // Allocate a new block.
+    return new BlockList(size);
+  }
+
   // 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);
-  }
   block->next = nullptr;
-
+  VALGRIND_MAKE_MEM_UNDEFINED(block->get(), size);
-  MemBlock result;
+  return block;
-  result.data = BLOCKDATA(block);
-  result.internalData = block;
-
-  VALGRIND_MAKE_MEM_UNDEFINED(result.data, size);
-
-  return result;
 }
 
-void MemoryPool::release(const MemBlock &mb) {
+MemBlock::MemBlock(int size) : bl(nullptr) { allocate(size); }
-  pthread_mutex_lock(&gMPoolMutex);
 
-  auto *block = (BlockList *)mb.internalData;
+MemBlock::~MemBlock() {
-
+  if (bl == nullptr) {
-  // just to be sure there's nothing important left in buffers..
+    return;
-  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);
   }
 
-void MemoryPool::destroyAll() {
+  bl->clear();
-  pthread_mutex_lock(&gMPoolMutex);
+  bl->next = gMemPool;
+  gMemPool = bl;
+  bl = nullptr;
+  data = nullptr;
+}
 
-  BlockList *block = gMemPool;
+void MemBlock::allocate(int size) {
-  gMemPool = nullptr;
+  if (bl != nullptr) {
+    if (size <= bl->size()) {
+      return;
+    }
 
-  pthread_mutex_unlock(&gMPoolMutex);
+    // Return existing block.
+    bl->clear();
+    bl->next = gMemPool;
+    gMemPool = bl;
+  }
 
-  while (block != nullptr) {
+  bl = allocateBlock(size);
-    BlockList *next = block->next;
+  data = bl->get();
+}
 
-    freeBlock(block);
+void MemBlock::freeAll() {
-    block = next;
+  while (gMemPool != nullptr) {
+    BlockList *next = gMemPool->next;
+
+    delete gMemPool;
+    gMemPool = next;
   }
 }
 

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;
-
+  BlockList *bl;
-  void *internalData;
-
-  MemBlock();
 };
 
-inline MemBlock::MemBlock() : data(0), internalData(0) {}
+inline bool MemBlock::valid() const {
+  return bl != nullptr;
+}
 
-/*
+inline unsigned char *MemBlock::get() const {
-    Memory Pool for fixed sized objects.
+  return data;
-
-    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();
 }
 
 }  // namespace encfs

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;
 }
 

encfs/SSL_Cipher.cpp

@@ -168,7 +168,8 @@ static Interface CAMELLIAInterface("ssl/camellia",3, 0, 2);
 static Range CAMELLIAKeyRange(128, 256, 64);
 static Range CAMELLIABlockRange(64, 4096, 16);
 
-static std::shared_ptr<Cipher> NewCAMELLIACipher(const Interface &iface, int keyLen) {
+static std::shared_ptr<Cipher> NewCAMELLIACipher(const Interface &iface,
+                                                 int keyLen) {
   if (keyLen <= 0) keyLen = 192;
 
   keyLen = CAMELLIAKeyRange.closest(keyLen);
@@ -197,12 +198,11 @@ static std::shared_ptr<Cipher> NewCAMELLIACipher(const Interface &iface, int key
 }
 
 static bool CAMELLIA_Cipher_registered =
-    Cipher::Register("CAMELLIA","16 byte block cipher", CAMELLIAInterface, CAMELLIAKeyRange,
+    Cipher::Register("CAMELLIA", "16 byte block cipher", CAMELLIAInterface,
-            CAMELLIABlockRange, NewCAMELLIACipher );
+                     CAMELLIAKeyRange, CAMELLIABlockRange, NewCAMELLIACipher);
 
 #endif
 
-
 #ifndef OPENSSL_NO_BF
 
 static Range BFKeyRange(128, 256, 32);

encfs/main.cpp

@@ -724,7 +724,7 @@ int main(int argc, char *argv[]) {
   rootInfo.reset();
   ctx->setRoot(std::shared_ptr<DirNode>());
 
-  MemoryPool::destroyAll();
+  MemBlock::freeAll();
   openssl_shutdown(encfsArgs->isThreaded);
 
   return returnCode;

encfs/test.cpp

@@ -54,41 +54,39 @@ const int FSBlockSize = 256;
 static int checkErrorPropogation(const std::shared_ptr<Cipher> &cipher,
                                  int size, int byteToChange,
                                  const CipherKey &key) {
-  MemBlock orig = MemoryPool::allocate(size);
+  MemBlock orig(size);
-  MemBlock data = MemoryPool::allocate(size);
+  MemBlock data(size);
 
   for (int i = 0; i < size; ++i) {
     unsigned char tmp = rand();
-    orig.data[i] = tmp;
+    orig.get()[i] = tmp;
-    data.data[i] = tmp;
+    data.get()[i] = tmp;
   }
 
-  if (size != FSBlockSize)
+  if (size != FSBlockSize) {
-    cipher->streamEncode(data.data, size, 0, key);
+    cipher->streamEncode(data.get(), size, 0, key);
-  else
+  } else {
-    cipher->blockEncode(data.data, size, 0, key);
+    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();
+      data.get()[byteToChange] = rand();
-    } while (data.data[byteToChange] == previousValue);
+    } 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;
 }