- Fix for reading empty files out of bounds

- Fixed race condition (#224 and #243)

ChangeLog.txt

@@ -2,6 +2,8 @@ Version 0.9.10 (unreleased)
 --------------
 Fixed bugs:
 * Fixed occasional deadlock (https://github.com/cryfs/cryfs/issues/64)
+* Fix for reading empty files out of bounds
+* Fixed race condition (https://github.com/cryfs/cryfs/issues/224)
 
 Version 0.9.9
 --------------

src/blobstore/implementations/onblocks/BlobOnBlocks.cpp

@@ -26,22 +26,29 @@ BlobOnBlocks::~BlobOnBlocks() {
 }
 
 uint64_t BlobOnBlocks::size() const {
+  std::unique_lock<std::mutex> lock(_datatree->mutex());
+  return _size();
+}
+
+uint64_t BlobOnBlocks::_size() const {
   if (_sizeCache == boost::none) {
     _sizeCache = _datatree->numStoredBytes();
   }
   return *_sizeCache;
 }
 
 void BlobOnBlocks::resize(uint64_t numBytes) {
+  std::unique_lock<std::mutex> lock(_datatree->mutex());
+
   _datatree->resizeNumBytes(numBytes);
   _sizeCache = numBytes;
 }
 
-void BlobOnBlocks::traverseLeaves(uint64_t beginByte, uint64_t sizeBytes, function<void (uint64_t, DataLeafNode *leaf, uint32_t, uint32_t)> func) const {
+void BlobOnBlocks::_traverseLeaves(uint64_t beginByte, uint64_t sizeBytes, function<void (uint64_t, DataLeafNode *leaf, uint32_t, uint32_t)> func) const {
   uint64_t endByte = beginByte + sizeBytes;
   uint32_t firstLeaf = beginByte / _datatree->maxBytesPerLeaf();
   uint32_t endLeaf = utils::ceilDivision(endByte, _datatree->maxBytesPerLeaf());
-  bool writingOutside = size() < endByte; // TODO Calling size() is slow because it has to traverse the tree
+  bool writingOutside = _size() < endByte; // TODO Calling size() is slow because it has to traverse the tree
   _datatree->traverseLeaves(firstLeaf, endLeaf, [&func, beginByte, endByte, endLeaf, writingOutside](DataLeafNode *leaf, uint32_t leafIndex) {
     uint64_t indexOfFirstLeafByte = leafIndex * leaf->maxStoreableBytes();
     uint32_t dataBegin = utils::maxZeroSubtraction(beginByte, indexOfFirstLeafByte);
@@ -59,41 +66,70 @@ void BlobOnBlocks::traverseLeaves(uint64_t beginByte, uint64_t sizeBytes, functi
 }
 
 Data BlobOnBlocks::readAll() const {
+  std::unique_lock<std::mutex> lock(_datatree->mutex());
+
   //TODO Querying size is inefficient. Is this possible without a call to size()?
-  uint64_t count = size();
+  uint64_t count = _size();
   Data result(count);
   _read(result.data(), 0, count);
   return result;
 }
 
 void BlobOnBlocks::read(void *target, uint64_t offset, uint64_t count) const {
-  ASSERT(offset <= size() && offset + count <= size(), "BlobOnBlocks::read() read outside blob. Use BlobOnBlocks::tryRead() if this should be allowed.");
-  uint64_t read = tryRead(target, offset, count);
-  ASSERT(read == count, "BlobOnBlocks::read() couldn't read all requested bytes. Use BlobOnBlocks::tryRead() if this should be allowed.");
+  std::unique_lock<std::mutex> lock(_datatree->mutex());
+
+  if(offset > _size() || offset + count > _size()) {
+      throw std::runtime_error("BlobOnBlocks::read() read outside blob. Use BlobOnBlocks::tryRead() if this should be allowed.");
+  }
+  uint64_t read = _tryRead(target, offset, count);
+  if(read != count) {
+      throw std::runtime_error("BlobOnBlocks::read() couldn't read all requested bytes. Use BlobOnBlocks::tryRead() if this should be allowed.");
+  }
 }
 
 uint64_t BlobOnBlocks::tryRead(void *target, uint64_t offset, uint64_t count) const {
+    std::unique_lock<std::mutex> lock(_datatree->mutex());
+    return _tryRead(target, offset, count);
+}
+
+uint64_t BlobOnBlocks::_tryRead(void *target, uint64_t offset, uint64_t count) const {
+  if (_size() <= offset) {
+    return 0;
+  }
+
   //TODO Quite inefficient to call size() here, because that has to traverse the tree
-  uint64_t realCount = std::max(UINT64_C(0), std::min(count, size()-offset));
+  uint64_t realCount = std::max(INT64_C(0), std::min(static_cast<int64_t>(count), static_cast<int64_t>(_size())-static_cast<int64_t>(offset)));
   _read(target, offset, realCount);
   return realCount;
 }
 
 void BlobOnBlocks::_read(void *target, uint64_t offset, uint64_t count) const {
-  traverseLeaves(offset, count, [target, offset] (uint64_t indexOfFirstLeafByte, const DataLeafNode *leaf, uint32_t leafDataOffset, uint32_t leafDataSize) {
+  if (count == 0) {
+    return;
+  }
+
+  _traverseLeaves(offset, count, [target, offset] (uint64_t indexOfFirstLeafByte, const DataLeafNode *leaf, uint32_t leafDataOffset, uint32_t leafDataSize) {
       //TODO Simplify formula, make it easier to understand
       leaf->read((uint8_t*)target + indexOfFirstLeafByte - offset + leafDataOffset, leafDataOffset, leafDataSize);
   });
 }
 
 void BlobOnBlocks::write(const void *source, uint64_t offset, uint64_t count) {
-  traverseLeaves(offset, count, [source, offset] (uint64_t indexOfFirstLeafByte, DataLeafNode *leaf, uint32_t leafDataOffset, uint32_t leafDataSize) {
+  if (count == 0) {
+    return;
+  }
+
+  std::unique_lock<std::mutex> lock(_datatree->mutex());
+
+  _traverseLeaves(offset, count, [source, offset] (uint64_t indexOfFirstLeafByte, DataLeafNode *leaf, uint32_t leafDataOffset, uint32_t leafDataSize) {
     //TODO Simplify formula, make it easier to understand
     leaf->write((uint8_t*)source + indexOfFirstLeafByte - offset + leafDataOffset, leafDataOffset, leafDataSize);
   });
 }
 
 void BlobOnBlocks::flush() {
+  std::unique_lock<std::mutex> lock(_datatree->mutex());
+
   _datatree->flush();
 }
 
@@ -102,6 +138,8 @@ const Key &BlobOnBlocks::key() const {
 }
 
 unique_ref<DataTreeRef> BlobOnBlocks::releaseTree() {
+  std::unique_lock<std::mutex> lock(_datatree->mutex());
+
   return std::move(_datatree);
 }
 

src/blobstore/implementations/onblocks/BlobOnBlocks.h

@@ -37,8 +37,11 @@ class BlobOnBlocks final: public Blob {
 
 private:
 
+  uint64_t _size() const;
+
   void _read(void *target, uint64_t offset, uint64_t count) const;
-  void traverseLeaves(uint64_t offsetBytes, uint64_t sizeBytes, std::function<void (uint64_t, datanodestore::DataLeafNode *, uint32_t, uint32_t)>) const;
+  uint64_t _tryRead(void *target, uint64_t offset, uint64_t size) const;
+  void _traverseLeaves(uint64_t offsetBytes, uint64_t sizeBytes, std::function<void (uint64_t, datanodestore::DataLeafNode *, uint32_t, uint32_t)>) const;
 
   cpputils::unique_ref<parallelaccessdatatreestore::DataTreeRef> _datatree;
   mutable boost::optional<uint64_t> _sizeCache;

src/blobstore/implementations/onblocks/datatreestore/DataTree.cpp

@@ -158,6 +158,10 @@ uint32_t DataTree::_numLeaves(const DataNode &node) const {
 }
 
 void DataTree::traverseLeaves(uint32_t beginIndex, uint32_t endIndex, function<void (DataLeafNode*, uint32_t)> func) {
+  if (endIndex <= beginIndex) {
+      return;
+  }
+
   //TODO Can we traverse in parallel?
   unique_lock<shared_mutex> lock(_mutex); //TODO Only lock when resizing. Otherwise parallel read/write to a blob is not possible!
   ASSERT(beginIndex <= endIndex, "Invalid parameters");

src/blobstore/implementations/onblocks/datatreestore/DataTree.h

@@ -38,7 +38,14 @@ class DataTree final {
 
   void flush() const;
 
+  // This is a hack to fix a race condition. This is only done in the 0.9 release branch to workaround the issue,
+  // the develop branch and 0.10 release series have a proper fix.
+  std::mutex& mutex() const {
+      return _outerMutex;
+  }
+
 private:
+  mutable std::mutex _outerMutex;
   mutable boost::shared_mutex _mutex;
   datanodestore::DataNodeStore *_nodeStore;
   cpputils::unique_ref<datanodestore::DataNode> _rootNode;

src/blobstore/implementations/onblocks/parallelaccessdatatreestore/DataTreeRef.h

@@ -41,6 +41,12 @@ class DataTreeRef final: public parallelaccessstore::ParallelAccessStore<datatre
     return _baseTree->flush();
   }
 
+  // This is a hack to fix a race condition. This is only done in the 0.9 release branch to workaround the issue,
+  // the develop branch and 0.10 release series have a proper fix.
+  std::mutex& mutex() const {
+    return _baseTree->mutex();
+  }
+
 private:
 
   datatreestore::DataTree *_baseTree;

test/blobstore/implementations/onblocks/BlobReadWriteTest.cpp

@@ -63,6 +63,92 @@ TEST_F(BlobReadWriteTest, WritingCloseTo16ByteLimitDoesntDestroySize) {
   EXPECT_EQ(32780u, blob->size());
 }
 
+TEST_F(BlobReadWriteTest, givenEmptyBlob_whenTryReadInFirstLeaf_thenFails) {
+  Data data(5);
+  size_t read = blob->tryRead(data.data(), 3, 5);
+  EXPECT_EQ(0, read);
+}
+
+TEST_F(BlobReadWriteTest, givenEmptyBlob_whenTryReadInLaterLeaf_thenFails) {
+  Data data(5);
+  size_t read = blob->tryRead(data.data(), 2*LAYOUT.maxBytesPerLeaf(), 5);
+  EXPECT_EQ(0, read);
+}
+
+TEST_F(BlobReadWriteTest, givenEmptyBlob_whenReadInFirstLeaf_thenFails) {
+  Data data(5);
+  EXPECT_ANY_THROW(
+          blob->read(data.data(), 3, 5)
+  );
+}
+
+TEST_F(BlobReadWriteTest, givenEmptyBlob_whenReadInLaterLeaf_thenFails) {
+  Data data(5);
+  EXPECT_ANY_THROW(
+          blob->read(data.data(), 2*LAYOUT.maxBytesPerLeaf(), 5)
+  );
+}
+
+TEST_F(BlobReadWriteTest, givenEmptyBlob_whenReadAll_thenReturnsZeroSizedData) {
+  Data data = blob->readAll();
+  EXPECT_EQ(0, data.size());
+}
+
+TEST_F(BlobReadWriteTest, givenEmptyBlob_whenWrite_thenGrows) {
+  Data data(5);
+  blob->write(data.data(), 4, 5);
+  EXPECT_EQ(9, blob->size());
+}
+
+TEST_F(BlobReadWriteTest, givenEmptyBlob_whenWriteZeroBytes_thenDoesntGrow) {
+  Data data(5);
+  blob->write(data.data(), 4, 0);
+  EXPECT_EQ(0, blob->size());;
+}
+
+TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenTryReadInFirstLeaf_thenFails) {
+  Data data(5);
+  size_t read = blob->tryRead(data.data(), 3, 5);
+  EXPECT_EQ(0, read);
+}
+
+TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenTryReadInLaterLeaf_thenFails) {
+  Data data(5);
+  size_t read = blob->tryRead(data.data(), 2*LAYOUT.maxBytesPerLeaf(), 5);
+  EXPECT_EQ(0, read);
+}
+
+TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenReadInFirstLeaf_thenFails) {
+  Data data(5);
+  EXPECT_ANY_THROW(
+          blob->read(data.data(), 3, 5)
+  );
+}
+
+TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenReadInLaterLeaf_thenFails) {
+  Data data(5);
+  EXPECT_ANY_THROW(
+          blob->read(data.data(), 2*LAYOUT.maxBytesPerLeaf(), 5)
+  );
+}
+
+TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenReadAll_thenReturnsZeroSizedData) {
+  Data data = blob->readAll();
+  EXPECT_EQ(0, data.size());
+}
+
+TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenWrite_thenGrows) {
+  Data data(5);
+  blob->write(data.data(), 4, 5);
+  EXPECT_EQ(9, blob->size());
+}
+
+TEST_F(BlobReadWriteTest, givenBlobResizedToZero_whenWriteZeroBytes_thenDoesntGrow) {
+  Data data(5);
+  blob->write(data.data(), 4, 0);
+  EXPECT_EQ(0, blob->size());
+}
+
 struct DataRange {
   size_t blobsize;
   off_t offset;