fix a bug that allowed torn reads, include a test to verify the fix (…

…covered by dco/Kalani_Thielen.md)

include/hobbes/fregion.H

@@ -653,17 +653,19 @@ inline fregion& mappedFileRegion(imagefile* f, file_pageindex_t page, size_t pag
 
 // allocate a region of this file as mapped memory
 inline char* mapFileData(imagefile* f, size_t fpos, size_t sz) {
-  file_pageindex_t page   = fpos / f->page_size;
-  uint16_t         offset = fpos % f->page_size;
+  file_pageindex_t pagei  = fpos        / f->page_size;
+  file_pageindex_t pagef  = (fpos + sz) / f->page_size;
+
+  assert(pagef >= pagei);
 
   // get the mapped region where this data lives
   // and increment its use count
-  fregion& r = mappedFileRegion(f, page, pageCount(f, sz));
+  fregion& r = mappedFileRegion(f, pagei, 1 + pagef - pagei);
   r.used += sz;
 
   // the result will be offset from the base of the mapped page
   // (plus any intervening pages from the base of the mapping to the page for this data)
-  char* result = r.base + (f->page_size * (page - r.base_page)) + offset;
+  char* result = r.base + (f->page_size * (pagei - r.base_page)) + (fpos % f->page_size);
 
   // remember where this allocated data came from (in case we want to release it later)
   falloc& fa = f->allocs[result];
@@ -855,11 +857,11 @@ inline void readFile(imagefile* f, uint16_t minVersion, uint16_t maxVersion) {
 }
 
 // open a file, or create it if necessary
-inline imagefile* openFile(const std::string& fname, bool readonly, uint16_t minVersion = HFREGION_CURRENT_FILE_FORMAT_VERSION, uint16_t maxVersion = HFREGION_CURRENT_FILE_FORMAT_VERSION) {
+inline imagefile* openFile(const std::string& fname, bool readonly, uint16_t minVersion = HFREGION_CURRENT_FILE_FORMAT_VERSION, uint16_t maxVersion = HFREGION_CURRENT_FILE_FORMAT_VERSION, size_t mmapPageMultiple = 262144 /* 1GB */) {
   imagefile* f        = new imagefile();
   f->path             = fname;
   f->readonly         = readonly;
-  f->mmapPageMultiple = 50*262144;    // map in 50GiB increments
+  f->mmapPageMultiple = mmapPageMultiple;
 
   try {
     // open the file
@@ -1884,7 +1886,7 @@ template <typename ... Wss>
 template <typename Ws, typename ... Wss>
   struct seqVar<Ws, Wss...> {
     static void stepEnc(ty::Variant::Ctors* cs, size_t* c, const Ws& s, const Wss& ... wss) {
-      cs->push_back(ty::Variant::Ctor(s.name(), *c, ty::fileRef(s.typeDefinition())));
+      cs->push_back(ty::Variant::Ctor(s.name(), *c, ty::fileRef(s.typeDef())));
       ++*c;
       seqVar<Wss...>::stepEnc(cs, c, wss...);
     }
@@ -1916,6 +1918,8 @@ template <typename ... Wss>
   };
 class writer {
 public:
+  writer(imagefile* f) : f(f) {
+  }
   writer(const std::string& fname) : f(openFile(fname, false)) {
   }
   ~writer() {
@@ -2168,8 +2172,12 @@ public:
       if (v == this->logDef.varDef.end()) {
         throw std::runtime_error("Constructor undefined in ordering: " + std::string(n));
       }
-      if (ty::encoding(v->second.second) != ty::encoding(store<T>::storeType())) {
-        throw std::runtime_error("Constructor '" + n + "' defined in ordering, but not with type " + ty::show(store<T>::storeType()));
+      if (ty::encoding(v->second.second) != ty::encoding(ty::fileRef(store<T>::storeType()))) {
+        throw std::runtime_error(
+          "Constructor '" + n + "' defined in ordering with inconsistent type.\n" + 
+          "  Expected: " + ty::show(store<T>::storeType()) + "\n" +
+          "  Actual:   " + ty::show(v->second.second)
+        );
       }
 
       // bind a function to process values out of this ordering
@@ -2217,9 +2225,13 @@ private:
     }
 
     LogDef r;
-    r.tdesc = ty::decode(b->second.type);
+    r.tdesc = maybeStoredBatchType(b->second.type);
     r.b     = &b->second;
 
+    if (!r.tdesc) {
+      throw std::runtime_error("File does not define '" + seqname + "' as a series.");
+    }
+
     if (r.tdesc->tid == PRIV_HPPF_TYCTOR_VARIANT) {
       for (const auto& ctor : reinterpret_cast<ty::Variant*>(r.tdesc.get())->ctors) {
         r.varDef[ctor.at<0>()] = std::pair<uint32_t, ty::desc>(ctor.at<1>(), ctor.at<2>());
@@ -2232,6 +2244,8 @@ private:
 };
 class reader {
 public:
+  reader(imagefile* f) : f(f) {
+  }
   reader(const std::string& fname) : f(openFile(fname, true)) {
   }
   ~reader() {

include/hobbes/reflect.H

@@ -28,18 +28,9 @@
 #include <fcntl.h>
 #include <unistd.h>
 
-// macOS doesn't support fallocate, simulate it
+// macOS doesn't support fallocate
 inline int posix_fallocate(int fd, off_t o, off_t dsz) {
-  fstore_t store = {F_ALLOCATECONTIG, F_PEOFPOSMODE, 0, o+dsz, 0};
-  int r = ::fcntl(fd, F_PREALLOCATE, &store);
-  if (r == -1) {
-    store.fst_flags = F_ALLOCATEALL;
-    r = fcntl(fd, F_PREALLOCATE, &store);
-  }
-  if (r != -1) {
-    r = ftruncate(fd, o+dsz);
-  }
-  return (r!=-1) ? 0 : -1;
+  return ftruncate(fd, o+dsz);
 }
 #endif
 

test/Storage.C

@@ -920,3 +920,61 @@ TEST(Storage, KeySeries) {
   }
 }
 
+// this test was added after some analysis found a case where "torn reads" could happen
+// (where a partial map is considered total for some value that we want to read, though it
+// actually is not and so an attempt to read the whole value causes a crash)
+//
+// this test was verified to reproduce the bug
+DEFINE_VARIANT(
+  Tick,
+  (bob,   int),
+  (frank, int),
+  (steve, std::string)
+);
+DEFINE_STRUCT(
+  TTick,
+  (Tick, t0),
+  (Tick, t1),
+  (Tick, t2)
+);
+TEST(Storage, AvoidTornRead) {
+  std::string fname = mkFName();
+  try {
+    hobbes::fregion::writer f(hobbes::fregion::openFile(fname, false, HFREGION_CURRENT_FILE_FORMAT_VERSION, HFREGION_CURRENT_FILE_FORMAT_VERSION, 1));
+    size_t batchsize = 1234;
+    auto& s = f.series<TTick>("tticks", batchsize);
+    f.recordOrdering("log", s);
+
+    // read now
+    std::ostringstream ss;
+
+    hobbes::fregion::reader rf(hobbes::fregion::openFile(fname, true, HFREGION_CURRENT_FILE_FORMAT_VERSION, HFREGION_CURRENT_FILE_FORMAT_VERSION, 1));
+    auto rlog = rf.ordering("log");
+    rlog.match<TTick>("tticks", [&](const TTick& tt) {
+      ss << tt << "\n";
+    });
+
+    // then dump two batches
+    for (size_t i = 0; i < 2; ++i) {
+      for (size_t i = 0; i < batchsize; ++i) {
+        TTick tt;
+        tt.t0 = Tick::bob(i);
+        tt.t1 = Tick::frank(i);
+        tt.t2 = Tick::bob(i);
+        s(tt);
+      }
+    }
+
+    // now try to read, with the torn read bug it will crash
+    while (rlog.next());
+
+    // if we get here, we are good
+    EXPECT_TRUE(ss.str().size() > 0);
+
+    unlink(fname.c_str());
+  } catch (...) {
+    unlink(fname.c_str());
+    throw;
+  }
+}
+