[heap] Fix an out-of-bounds access in the marking bitmap

Deserializer can trigger OOB read in the marking bitmap inside the
RegisterDeserializedObjectsForBlackAllocation function. This happens
for example if an internalized string is deserialized as the last object
on a page and is the turned into a thin-string leaving a one-word filler
at the end of the page. In such a case IsBlack(filler) will try to fetch
a cell outside the marking bitmap.

The fix is to increase the size of the marking bitmap by one cell, so
that it is always safe to query markbits of any object on a page.

Bug: chromium:978156
Change-Id: If3c74e4f97d2caeb3c3f37a4147f38dea5f0e5a8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2152838
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67223}

src/heap/marking.cc

@@ -7,6 +7,8 @@
 namespace v8 {
 namespace internal {
 
+const size_t Bitmap::kSize = Bitmap::CellsCount() * Bitmap::kBytesPerCell;
+
 template <>
 bool ConcurrentBitmap<AccessMode::NON_ATOMIC>::AllBitsSetInRange(
     uint32_t start_index, uint32_t end_index) {
@@ -77,7 +79,7 @@ class CellPrinter {
  public:
   CellPrinter() : seq_start(0), seq_type(0), seq_length(0) {}
 
-  void Print(uint32_t pos, uint32_t cell) {
+  void Print(size_t pos, uint32_t cell) {
     if (cell == seq_type) {
       seq_length++;
       return;
@@ -92,14 +94,14 @@ class CellPrinter {
       return;
     }
 
-    PrintF("%d: ", pos);
+    PrintF("%zu: ", pos);
     PrintWord(cell);
     PrintF("\n");
   }
 
   void Flush() {
     if (seq_length > 0) {
-      PrintF("%d: %dx%d\n", seq_start, seq_type == 0 ? 0 : 1,
+      PrintF("%zu: %dx%zu\n", seq_start, seq_type == 0 ? 0 : 1,
              seq_length * Bitmap::kBitsPerCell);
       seq_length = 0;
     }
@@ -108,9 +110,9 @@ class CellPrinter {
   static bool IsSeq(uint32_t cell) { return cell == 0 || cell == 0xFFFFFFFF; }
 
  private:
-  uint32_t seq_start;
+  size_t seq_start;
   uint32_t seq_type;
-  uint32_t seq_length;
+  size_t seq_length;
 };
 
 }  // anonymous namespace
@@ -118,7 +120,7 @@ class CellPrinter {
 template <>
 void ConcurrentBitmap<AccessMode::NON_ATOMIC>::Print() {
   CellPrinter printer;
-  for (int i = 0; i < CellsCount(); i++) {
+  for (size_t i = 0; i < CellsCount(); i++) {
     printer.Print(i, cells()[i]);
   }
   printer.Flush();
@@ -127,7 +129,7 @@ void ConcurrentBitmap<AccessMode::NON_ATOMIC>::Print() {
 
 template <>
 bool ConcurrentBitmap<AccessMode::NON_ATOMIC>::IsClean() {
-  for (int i = 0; i < CellsCount(); i++) {
+  for (size_t i = 0; i < CellsCount(); i++) {
     if (cells()[i] != 0) {
       return false;
     }

src/heap/marking.h

@@ -98,16 +98,18 @@ class V8_EXPORT_PRIVATE Bitmap {
   static const uint32_t kBytesPerCell = kBitsPerCell / kBitsPerByte;
   static const uint32_t kBytesPerCellLog2 = kBitsPerCellLog2 - kBitsPerByteLog2;
 
-  static const size_t kLength = (1 << kPageSizeBits) >> (kTaggedSizeLog2);
-
-  static const size_t kSize = (1 << kPageSizeBits) >>
-                              (kTaggedSizeLog2 + kBitsPerByteLog2);
-
-  static int CellsForLength(int length) {
+  // The length is the number of bits in this bitmap. (+1) accounts for
+  // the case where the markbits are queried for a one-word filler at the
+  // end of the page.
+  static const size_t kLength = ((1 << kPageSizeBits) >> kTaggedSizeLog2) + 1;
+  // The size of the bitmap in bytes is CellsCount() * kBytesPerCell.
+  static const size_t kSize;
+
+  static constexpr size_t CellsForLength(int length) {
     return (length + kBitsPerCell - 1) >> kBitsPerCellLog2;
   }
 
-  int CellsCount() { return CellsForLength(kLength); }
+  static constexpr size_t CellsCount() { return CellsForLength(kLength); }
 
   V8_INLINE static uint32_t IndexToCell(uint32_t index) {
     return index >> kBitsPerCellLog2;

test/cctest/heap/test-heap.cc

@@ -6933,6 +6933,41 @@ TEST(NoCodeRangeInJitlessMode) {
       CcTest::i_isolate()->heap()->memory_allocator()->code_range().is_empty());
 }
 
+TEST(Regress978156) {
+  if (!FLAG_incremental_marking) return;
+  ManualGCScope manual_gc_scope;
+  CcTest::InitializeVM();
+
+  HandleScope handle_scope(CcTest::i_isolate());
+  Heap* heap = CcTest::i_isolate()->heap();
+
+  // 1. Ensure that the new space is empty.
+  CcTest::CollectGarbage(NEW_SPACE);
+  CcTest::CollectGarbage(NEW_SPACE);
+  // 2. Fill the first page of the new space with FixedArrays.
+  std::vector<Handle<FixedArray>> arrays;
+  i::heap::FillCurrentPage(heap->new_space(), &arrays);
+  // 3. Trim the last array by one word thus creating a one-word filler.
+  Handle<FixedArray> last = arrays.back();
+  CHECK_GT(last->length(), 0);
+  heap->RightTrimFixedArray(*last, 1);
+  // 4. Get the last filler on the page.
+  HeapObject filler = HeapObject::FromAddress(
+      MemoryChunk::FromHeapObject(*last)->area_end() - kTaggedSize);
+  HeapObject::FromAddress(last->address() + last->Size());
+  CHECK(filler.IsFiller());
+  // 5. Start incremental marking.
+  i::IncrementalMarking* marking = heap->incremental_marking();
+  if (marking->IsStopped()) {
+    marking->Start(i::GarbageCollectionReason::kTesting);
+  }
+  IncrementalMarking::MarkingState* marking_state = marking->marking_state();
+  // 6. Mark the filler black to access its two markbits. This triggers
+  // an out-of-bounds access of the marking bitmap in a bad case.
+  marking_state->WhiteToGrey(filler);
+  marking_state->GreyToBlack(filler);
+}
+
 }  // namespace heap
 }  // namespace internal
 }  // namespace v8

test/unittests/heap/bitmap-unittest.cc

@@ -41,7 +41,7 @@ TEST_F(NonAtomicBitmapTest, Cells) {
 }
 
 TEST_F(NonAtomicBitmapTest, CellsCount) {
-  int last_cell_index = bitmap()->CellsCount() - 1;
+  size_t last_cell_index = bitmap()->CellsCount() - 1;
   bitmap()->cells()[last_cell_index] = kBlackCell;
   // Manually verify on raw memory.
   uint8_t* raw = raw_bitmap();