Protect the Card Boundary table as often as possible to detect corrup…

…ting writes.

Summary:
Before this diff, we had inserted debugging code that "summarized" the card boundary tables of all old-gen segments at the end of GC, recording the portion that was summarized.  At the start of the next GC, we summarized the boundary tables again, making sure they hadn't changed.  We got an instance of the crash we're chasing in production with this instrumentation, and it indicated that the card boundary table had been corrupted.  But we don't know how.

This diff takes a next step: it uses memory protection on the boundary table in all times outside of legitimate modifications.  The hope is that whatever corrupting write is happening will be "caught in the act" by this.

Reviewed By: kodafb

Differential Revision: D17172877

fbshipit-source-id: 1292319f3f1be52af907d1df8e9da937deb0b724

include/hermes/VM/AlignedHeapSegment.h

@@ -200,23 +200,6 @@ class AlignedHeapSegment final {
   ///     this \c AlignedHeapSegment.
   inline bool contains(const void *ptr) const;
 
-#ifdef HERMES_EXTRA_DEBUG
-  /// Extra debugging: at the end of GC we "summarize" the card
-  /// boundary table.  That is, treat the portion currently in use as
-  /// a string, and takes its hash.  The card boundary table below the
-  /// card corresponding to "level()" should not change during mutator
-  /// operation, so at the beginning of the next GC, we verify that
-  /// this has is the same.
-  /// TODO(T48709128): remove these when the problem is diagnosed.
-
-  /// Summarize the card boundary table, and save the results.
-  void summarizeCardTableBoundaries();
-
-  /// Resummarize the card boundary table, and return whether the
-  /// result is the same.
-  bool checkSummarizedCardTableBoundaries() const;
-#endif
-
   /// Assumes that the segment's card object boundaries may not have been
   /// maintained, and recreates it, ensuring that it's valid.
   void recreateCardTableBoundaries();
@@ -368,17 +351,6 @@ class AlignedHeapSegment final {
 
   /// Pointer to the generation that owns this segment.
   GCGeneration *generation_{nullptr};
-
-#ifdef HERMES_EXTRA_DEBUG
-  /// Support summarization of the boundary table.
-  /// TODO(T48709128): remove this when the problem is diagnosed.
-
-  /// The level at the time we last summarized, or start(), if we
-  /// haven't previously summarized.
-  char *lastBoundarySummaryLevel_{start()};
-  /// The value of the last summary, or else 0 if there has been no summary.
-  size_t lastBoundarySummary_{0};
-#endif
 };
 
 AllocResult AlignedHeapSegment::alloc(uint32_t size) {

include/hermes/VM/CardTableNC.h

@@ -165,10 +165,13 @@ class CardTable {
     return boundaries_[index];
   }
 
-  /// Treat the portion of the card boundary table corresponding to the given
-  /// limits as a string, and return the hash of that string.
+  /// These methods protect and unprotect, respectively, the memory
+  /// that comprises the card boundary table.  They require that the
+  /// start of the boundary table is page-aligned, and its size is a
+  /// multiple of the page size.
   /// TODO(T48709128): remove this when the problem is diagnosed.
-  size_t summarizeBoundaries(char *start, char *end) const;
+  void protectBoundaryTable();
+  void unprotectBoundaryTable();
 #endif // HERMES_EXTRA_DEBUG
 
 #ifdef HERMES_SLOW_DEBUG

include/hermes/VM/OldGenNC.h

@@ -203,6 +203,11 @@ class OldGen : public GCGeneration {
     }
   };
 
+  /// An allocation yielded \p alloc, and \p nextAlloc is one byte after the
+  /// end of the allocated object.  This allocation extended into a new card.
+  /// Update the card boundary table.
+  void updateBoundariesAfterAlloc(char *alloc, char *nextAlloc);
+
   /// The current allocation position.  The first version may be used always;
   /// the availableDirect version may only be used when the generation owns its
   /// allocation context, but is faster.
@@ -281,14 +286,23 @@ class OldGen : public GCGeneration {
 #endif
 
 #ifdef HERMES_EXTRA_DEBUG
-  /// Summarize the card boundary tables of all segments, saving the results.
+  /// These methods protect and unprotect, respectively, the memory
+  /// that comprises the card boundary tables of all the segments in
+  /// the old generation.  They require that the starts of the
+  /// boundary tables are page-aligned, and the table size is a
+  /// multiple of the page size.
   /// TODO(T48709128): remove this when the problem is diagnosed.
-  void summarizeCardTableBoundaries();
-
-  /// For every segment, summarize up to the level when we last summarized, and
-  /// make sure the summary is the same.
+  void protectCardTableBoundaries();
+  void unprotectCardTableBoundaries();
+
+  /// These methods protect and unprotect, respectively, the memory
+  /// that comprises the card boundary table of the active segment of
+  /// the old generation.  They require that the start of the boundary
+  /// tables are page-aligned, and the table size is a multiple of the
+  /// page size.
   /// TODO(T48709128): remove this when the problem is diagnosed.
-  void checkSummarizedCardTableBoundaries() const;
+  void protectActiveSegCardTableBoundaries();
+  void unprotectActiveSegCardTableBoundaries();
 #endif
 
   /// Static override of GCGeneration::didFinishGC().
@@ -444,8 +458,7 @@ AllocResult OldGen::allocRaw(uint32_t size, HasFinalizer hasFinalizer) {
   char *resPtr = reinterpret_cast<char *>(result.ptr);
   char *nextAllocPtr = activeSegment().level();
   if (cardBoundary_.address() < nextAllocPtr) {
-    activeSegment().cardTable().updateBoundaries(
-        &cardBoundary_, resPtr, nextAllocPtr);
+    updateBoundariesAfterAlloc(resPtr, nextAllocPtr);
   }
 
   return {result.ptr, true};

lib/VM/gcs/AlignedHeapSegment.cpp

@@ -442,18 +442,6 @@ void AlignedHeapSegment::recreateCardTableBoundaries() {
   }
 }
 
-#ifdef HERMES_EXTRA_DEBUG
-void AlignedHeapSegment::summarizeCardTableBoundaries() {
-  lastBoundarySummary_ = cardTable().summarizeBoundaries(start(), level());
-  lastBoundarySummaryLevel_ = level();
-}
-
-bool AlignedHeapSegment::checkSummarizedCardTableBoundaries() const {
-  return lastBoundarySummary_ ==
-      cardTable().summarizeBoundaries(start(), lastBoundarySummaryLevel_);
-}
-#endif
-
 #ifndef NDEBUG
 bool AlignedHeapSegment::dbgContainsLevel(const void *lvl) const {
   return contains(lvl) || lvl == hiLim();

lib/VM/gcs/CardTableNC.cpp

@@ -8,6 +8,8 @@
 
 #include "hermes/VM/CardTableNC.h"
 
+#include "hermes/Support/OSCompat.h"
+
 #include <string.h>
 #include <algorithm>
 #include <cassert>
@@ -134,24 +136,23 @@ GCCell *CardTable::firstObjForCard(unsigned index) const {
 }
 
 #ifdef HERMES_EXTRA_DEBUG
-size_t CardTable::summarizeBoundaries(char *start, char *end) const {
-  size_t startInd = addressToIndex(start);
-  size_t endInd = addressToIndex(end);
-  // If end is exactly at the boundary, we will not have set the
-  // boundary entry for endInd.  If end has crossed the boundary, we
-  // will.  Make endInd the non-inclusive uppert bound.
-  if (indexToAddress(endInd) != end) {
-    endInd++;
-  }
-  if (endInd == startInd) {
-    // We return zero in this case -- this matches the initial value of
-    // the summary.  So if nothing is allocated, the summary remains zero.
-    return 0;
-  }
-  std::hash<std::string> stringHash;
-  return stringHash(std::string(
-      reinterpret_cast<const char *>(&boundaries_[startInd]),
-      endInd - startInd));
+static void
+protectBoundaryTableWork(void *table, size_t sz, oscompat::ProtectMode mode) {
+  assert((reinterpret_cast<uintptr_t>(table) % oscompat::page_size()) == 0);
+  assert((sz % oscompat::page_size()) == 0);
+  bool res = oscompat::vm_protect(table, sz, mode);
+  (void)res;
+  assert(res);
+}
+
+void CardTable::protectBoundaryTable() {
+  protectBoundaryTableWork(
+      &boundaries_[0], kValidIndices, oscompat::ProtectMode::None);
+}
+
+void CardTable::unprotectBoundaryTable() {
+  protectBoundaryTableWork(
+      &boundaries_[0], kValidIndices, oscompat::ProtectMode::ReadWrite);
 }
 #endif // HERMES_EXTRA_DEBUG
 

lib/VM/gcs/GenGCNC.cpp

@@ -274,9 +274,9 @@ void GenGC::collect(bool canEffectiveOOM) {
     oom(make_error_code(OOMError::Effective));
 
 #ifdef HERMES_EXTRA_DEBUG
-  /// Check the card table boundary summary, to detect possible mutator writes.
+  /// Unprotect the card table boundary table, so we can updated it.
   /// TODO(T48709128): remove these when the problem is diagnosed.
-  oldGen_.checkSummarizedCardTableBoundaries();
+  oldGen_.unprotectCardTableBoundaries();
 #endif
 
   /// Yield, then reclaim, the allocation context.  (This is a noop
@@ -376,9 +376,10 @@ void GenGC::collect(bool canEffectiveOOM) {
 #endif
 
 #ifdef HERMES_EXTRA_DEBUG
-  /// Summarize the card table boundaries, to detect possible mutator writes.
+  /// Protect the card table boundary table, to detect corrupting mutator
+  /// writes.
   /// TODO(T48709128): remove these when the problem is diagnosed.
-  oldGen_.summarizeCardTableBoundaries();
+  oldGen_.protectCardTableBoundaries();
 #endif
 }
 

lib/VM/gcs/OldGenNC.cpp

@@ -794,6 +794,24 @@ AllocResult OldGen::allocRawSlow(uint32_t size, HasFinalizer hasFinalizer) {
   return allocRaw(size, hasFinalizer);
 }
 
+void OldGen::updateBoundariesAfterAlloc(char *alloc, char *nextAlloc) {
+#ifdef HERMES_EXTRA_DEBUG
+  // The allocation may update the boundary table of the old gen's
+  // active segment, so unprotect it (and reprotect below).
+  bool doUnprotect = !gc_->inGC();
+  if (doUnprotect) {
+    unprotectActiveSegCardTableBoundaries();
+  }
+#endif
+  activeSegment().cardTable().updateBoundaries(
+      &cardBoundary_, alloc, nextAlloc);
+#ifdef HERMES_EXTRA_DEBUG
+  if (doUnprotect) {
+    protectActiveSegCardTableBoundaries();
+  }
+#endif
+}
+
 #ifdef HERMES_SLOW_DEBUG
 void OldGen::checkWellFormed(const GC *gc) const {
   uint64_t totalExtSize = 0;
@@ -810,32 +828,25 @@ void OldGen::checkWellFormed(const GC *gc) const {
 #endif
 
 #ifdef HERMES_EXTRA_DEBUG
-void OldGen::summarizeCardTableBoundaries() {
+void OldGen::protectCardTableBoundaries() {
   forUsedSegments([](AlignedHeapSegment &segment) {
-    segment.summarizeCardTableBoundaries();
+    segment.cardTable().protectBoundaryTable();
   });
 }
 
-void OldGen::checkSummarizedCardTableBoundaries() const {
-  static unsigned numSummaryErrors = 0;
-  forUsedSegments([this](const AlignedHeapSegment &segment) {
-    if (!segment.checkSummarizedCardTableBoundaries()) {
-      numSummaryErrors++;
-      char detailBuffer[100];
-      snprintf(
-          detailBuffer,
-          sizeof(detailBuffer),
-          "CardObjectTable summary changed since last GC for "
-          "[%p, %p).  (Last of %d errors)",
-          segment.lowLim(),
-          segment.hiLim(),
-          numSummaryErrors);
-      hermesLog("HermesGC", "Error: %s.", detailBuffer);
-      // Record the OOM custom data with the crash manager.
-      gc_->crashMgr_->setCustomData("HermesGCBadCOTSummary", detailBuffer);
-    }
+void OldGen::unprotectCardTableBoundaries() {
+  forUsedSegments([](AlignedHeapSegment &segment) {
+    segment.cardTable().unprotectBoundaryTable();
   });
 }
+
+void OldGen::protectActiveSegCardTableBoundaries() {
+  activeSegment().cardTable().protectBoundaryTable();
+}
+
+void OldGen::unprotectActiveSegCardTableBoundaries() {
+  activeSegment().cardTable().unprotectBoundaryTable();
+}
 #endif
 
 void OldGen::didFinishGC() {

lib/VM/gcs/YoungGenNC.cpp

@@ -269,9 +269,10 @@ void YoungGen::collect() {
   GenGC::CollectionSection ygCollection(gc_, "YoungGen collection");
 
 #ifdef HERMES_EXTRA_DEBUG
-  /// Check the card table boundary summary, to detect possible mutator writes.
+  /// Protect the card table boundary table, to detect corrupting mutator
+  /// writes.
   /// TODO(T48709128): remove these when the problem is diagnosed.
-  nextGen_->checkSummarizedCardTableBoundaries();
+  nextGen_->unprotectCardTableBoundaries();
 #endif
 
   // Reset the number of consecutive full GCs, because we're about to do a young
@@ -415,9 +416,10 @@ void YoungGen::collect() {
 #endif
 
 #ifdef HERMES_EXTRA_DEBUG
-  /// Summarize the card table boundaries, to detect possible mutator writes.
+  /// Protect the card table boundary table, to detect corrupting mutator
+  /// writes.
   /// TODO(T48709128): remove these when the problem is diagnosed.
-  nextGen_->summarizeCardTableBoundaries();
+  nextGen_->protectCardTableBoundaries();
 #endif
 
   CrashManager::HeapInformation crashHeapInfo;