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

ulan · Commit Bot · commit 8e8a06fac9cc · 2020-04-20T09:07:57.000Z
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}
diff --git a/src/heap/marking.cc b/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,17 +110,17 @@ 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
 
 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;
     }
diff --git a/src/heap/marking.h b/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;
diff --git a/test/cctest/heap/test-heap.cc b/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
diff --git a/test/unittests/heap/bitmap-unittest.cc b/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();

Original file line number	Diff line number	Diff line change
`@@ -41,7 +41,7 @@ TEST_F(NonAtomicBitmapTest, Cells) {`
`41`	`41`	`}`
`42`	`42`
`43`	`43`	`TEST_F(NonAtomicBitmapTest, CellsCount) {`
`44`		`- int last_cell_index = bitmap()->CellsCount() - 1;`
	`44`	`+ size_t last_cell_index = bitmap()->CellsCount() - 1;`
`45`	`45`	`bitmap()->cells()[last_cell_index] = kBlackCell;`
`46`	`46`	`// Manually verify on raw memory.`
`47`	`47`	`uint8_t* raw = raw_bitmap();`