[heap] Mark RO_SPACE as read-only after deserialization

Adds MarkAsReadOnly and MarkAsReadWrite to ReadOnlySpace. The latter
is only usable with ReadOnlySpace::WritableScope to avoid the space
being left writable). MarkAsReadOnly updates the high water mark and
makes several previously mutating methods into no-ops.

Moves some writes to immutable objects out of the bootstrapper to
setup-heap-internal so they don't write to a read-only page.

Also avoid writing hashes to strings that already have the value set as
that invariably means writing to the "0" and "1" constant strings in
RO_SPACE.

Before serialization, it makes RO_SPACE writable again so that any
padding can be cleared before writing it.

Bug: v8:7464
Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
Change-Id: I22edc20dba7dde8943991a8fcaf87244af4490a3
Reviewed-on: https://chromium-review.googlesource.com/1014128
Commit-Queue: Dan Elphick <delphick@chromium.org>
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52943}

src/api.cc

@@ -761,28 +761,42 @@ StartupData SnapshotCreator::CreateBlob(
   CHECK(handle_checker.CheckGlobalAndEternalHandles());
 
   i::HeapIterator heap_iterator(isolate->heap());
-  while (i::HeapObject* current_obj = heap_iterator.next()) {
-    if (current_obj->IsJSFunction()) {
-      i::JSFunction* fun = i::JSFunction::cast(current_obj);
+  {
+    // Make RO_SPACE writable temporarily so the padding on its strings can be
+    // cleared.
+    i::ReadOnlySpace::WritableScope writable_read_only_scope(
+        isolate->heap()->read_only_space());
 
-      // Complete in-object slack tracking for all functions.
-      fun->CompleteInobjectSlackTrackingIfActive();
+    while (i::HeapObject* current_obj = heap_iterator.next()) {
+      if (current_obj->IsJSFunction()) {
+        i::JSFunction* fun = i::JSFunction::cast(current_obj);
 
-      // Also, clear out feedback vectors.
-      fun->feedback_cell()->set_value(isolate->heap()->undefined_value());
-    }
+        // Complete in-object slack tracking for all functions.
+        fun->CompleteInobjectSlackTrackingIfActive();
+
+        // Also, clear out feedback vectors.
+        fun->feedback_cell()->set_value(isolate->heap()->undefined_value());
+      }
 
-    // Clear out re-compilable data from all shared function infos. Any
-    // JSFunctions using these SFIs will have their code pointers reset by the
-    // partial serializer.
-    if (current_obj->IsSharedFunctionInfo() &&
-        function_code_handling == FunctionCodeHandling::kClear) {
-      i::SharedFunctionInfo* shared = i::SharedFunctionInfo::cast(current_obj);
-      if (shared->CanFlushCompiled()) {
-        shared->FlushCompiled();
+      // Clear out re-compilable data from all shared function infos. Any
+      // JSFunctions using these SFIs will have their code pointers reset by the
+      // partial serializer.
+      if (current_obj->IsSharedFunctionInfo() &&
+          function_code_handling == FunctionCodeHandling::kClear) {
+        i::SharedFunctionInfo* shared =
+            i::SharedFunctionInfo::cast(current_obj);
+        if (shared->CanFlushCompiled()) {
+          shared->FlushCompiled();
+        }
+        DCHECK(shared->HasCodeObject() || shared->HasBuiltinId() ||
+               shared->IsApiFunction());
+      }
+
+      if (current_obj->IsSeqOneByteString()) {
+        i::SeqOneByteString::cast(current_obj)->clear_padding();
+      } else if (current_obj->IsSeqTwoByteString()) {
+        i::SeqTwoByteString::cast(current_obj)->clear_padding();
       }
-      DCHECK(shared->HasCodeObject() || shared->HasBuiltinId() ||
-             shared->IsApiFunction());
     }
   }
 
@@ -6719,8 +6733,9 @@ bool v8::String::CanMakeExternal() {
   if (obj->IsExternalString()) return false;
 
   // Old space strings should be externalized.
-  i::Isolate* isolate = obj->GetIsolate();
-  return !isolate->heap()->new_space()->Contains(*obj);
+  i::Heap* heap = obj->GetIsolate()->heap();
+  return !heap->new_space()->Contains(*obj) &&
+         !heap->read_only_space()->Contains(*obj);
 }
 
 

src/bootstrapper.cc

@@ -3034,17 +3034,6 @@ void Genesis::InitializeGlobal(Handle<JSGlobalObject> global_object,
   }
 
   {  // -- M a p
-    {
-      Handle<String> index_string = isolate->factory()->zero_string();
-      uint32_t field =
-          StringHasher::MakeArrayIndexHash(0, index_string->length());
-      index_string->set_hash_field(field);
-
-      index_string = isolate->factory()->one_string();
-      field = StringHasher::MakeArrayIndexHash(1, index_string->length());
-      index_string->set_hash_field(field);
-    }
-
     Handle<JSFunction> js_map_fun =
         InstallFunction(global, "Map", JS_MAP_TYPE, JSMap::kSize, 0,
                         factory->the_hole_value(), Builtins::kMapConstructor);
@@ -4348,8 +4337,6 @@ void Genesis::InitializeGlobal_harmony_bigint() {
   bigint_fun->shared()->set_length(1);
   InstallWithIntrinsicDefaultProto(isolate(), bigint_fun,
                                    Context::BIGINT_FUNCTION_INDEX);
-  heap()->bigint_map()->SetConstructorFunctionIndex(
-      Context::BIGINT_FUNCTION_INDEX);
 
   // Install the properties of the BigInt constructor.
   // asUintN(bits, bigint)

src/heap/factory-inl.h

@@ -143,7 +143,8 @@ Handle<Object> Factory::NewURIError() {
 Handle<String> Factory::Uint32ToString(uint32_t value) {
   Handle<String> result = NumberToString(NewNumberFromUint(value));
 
-  if (result->length() <= String::kMaxArrayIndexSize) {
+  if (result->length() <= String::kMaxArrayIndexSize &&
+      result->hash_field() == String::kEmptyHashField) {
     uint32_t field = StringHasher::MakeArrayIndexHash(value, result->length());
     result->set_hash_field(field);
   }

src/heap/heap.cc

@@ -4796,6 +4796,7 @@ void Heap::NotifyDeserializationComplete() {
 #endif  // DEBUG
   }
 
+  read_only_space()->MarkAsReadOnly();
   deserialization_complete_ = true;
 }
 

src/heap/setup-heap-internal.cc

@@ -553,6 +553,9 @@ bool Heap::CreateInitialMaps() {
 #undef ALLOCATE_EMPTY_FIXED_TYPED_ARRAY
 
   DCHECK(!InNewSpace(empty_fixed_array()));
+
+  bigint_map()->SetConstructorFunctionIndex(Context::BIGINT_FUNCTION_INDEX);
+
   return true;
 }
 
@@ -847,6 +850,10 @@ void Heap::CreateInitialObjects() {
   set_deserialize_lazy_handler_wide(Smi::kZero);
   set_deserialize_lazy_handler_extra_wide(Smi::kZero);
 
+  // Evaluate the hash values which will then be cached in the strings.
+  isolate()->factory()->zero_string()->Hash();
+  isolate()->factory()->one_string()->Hash();
+
   // Initialize builtins constants table.
   set_builtins_constants_table(empty_fixed_array());
 

src/heap/spaces.cc

@@ -3196,6 +3196,28 @@ bool PagedSpace::RawSlowRefillLinearAllocationArea(int size_in_bytes) {
 void MapSpace::VerifyObject(HeapObject* object) { CHECK(object->IsMap()); }
 #endif
 
+void ReadOnlySpace::SetPermissionsForPages(PageAllocator::Permission access) {
+  const size_t page_size = MemoryAllocator::GetCommitPageSize();
+  const size_t area_start_offset = RoundUp(Page::kObjectStartOffset, page_size);
+  for (Page* page : *this) {
+    CHECK(SetPermissions(page->address() + area_start_offset,
+                         page->size() - area_start_offset, access));
+  }
+}
+
+void ReadOnlySpace::MarkAsReadOnly() {
+  DCHECK(!is_marked_read_only_);
+  FreeLinearAllocationArea();
+  is_marked_read_only_ = true;
+  SetPermissionsForPages(PageAllocator::kRead);
+}
+
+void ReadOnlySpace::MarkAsReadWrite() {
+  DCHECK(is_marked_read_only_);
+  SetPermissionsForPages(PageAllocator::kReadWrite);
+  is_marked_read_only_ = false;
+}
+
 Address LargePage::GetAddressToShrink(Address object_address,
                                       size_t object_size) {
   if (executable() == EXECUTABLE) {

src/heap/spaces.h

@@ -2894,8 +2894,33 @@ class MapSpace : public PagedSpace {
 
 class ReadOnlySpace : public PagedSpace {
  public:
+  class WritableScope {
+   public:
+    explicit WritableScope(ReadOnlySpace* space) : space_(space) {
+      space_->MarkAsReadWrite();
+    }
+
+    ~WritableScope() { space_->MarkAsReadOnly(); }
+
+   private:
+    ReadOnlySpace* space_;
+  };
+
   ReadOnlySpace(Heap* heap, AllocationSpace id, Executability executable)
       : PagedSpace(heap, id, executable) {}
+
+  ~ReadOnlySpace() {
+    // Mark as writable as tearing down the space writes to it.
+    // MarkAsReadWrite();
+  }
+
+  void MarkAsReadOnly();
+
+ private:
+  void MarkAsReadWrite();
+  void SetPermissionsForPages(PageAllocator::Permission access);
+
+  bool is_marked_read_only_ = false;
 };
 
 // -----------------------------------------------------------------------------

src/objects.cc

@@ -2674,6 +2674,7 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
   // Abort if size does not allow in-place conversion.
   if (size < ExternalString::kShortSize) return false;
   Heap* heap = GetHeap();
+  if (heap->read_only_space()->Contains(this)) return false;
   bool is_internalized = this->IsInternalizedString();
   bool has_pointers = StringShape(this).IsIndirect();
 

src/snapshot/serializer.cc

@@ -609,12 +609,6 @@ void Serializer<AllocatorT>::ObjectSerializer::Serialize() {
   if (object_->IsExternalString()) {
     SerializeExternalString();
     return;
-  } else if (object_->IsSeqOneByteString()) {
-    // Clear padding bytes at the end. Done here to avoid having to do this
-    // at allocation sites in generated code.
-    SeqOneByteString::cast(object_)->clear_padding();
-  } else if (object_->IsSeqTwoByteString()) {
-    SeqTwoByteString::cast(object_)->clear_padding();
   }
   if (object_->IsJSTypedArray()) {
     SerializeJSTypedArray();

test/cctest/test-strings.cc

@@ -1642,6 +1642,21 @@ GC_INSIDE_NEW_STRING_FROM_UTF8_SUB_STRING(
 
 #undef GC_INSIDE_NEW_STRING_FROM_UTF8_SUB_STRING
 
+TEST(HashArrayIndexStrings) {
+  CcTest::InitializeVM();
+  LocalContext context;
+  v8::HandleScope scope(CcTest::isolate());
+  i::Isolate* isolate = CcTest::i_isolate();
+
+  CHECK_EQ(StringHasher::MakeArrayIndexHash(0 /* value */, 1 /* length */) >>
+               Name::kHashShift,
+           isolate->factory()->zero_string()->Hash());
+
+  CHECK_EQ(StringHasher::MakeArrayIndexHash(1 /* value */, 1 /* length */) >>
+               Name::kHashShift,
+           isolate->factory()->one_string()->Hash());
+}
+
 }  // namespace test_strings
 }  // namespace internal
 }  // namespace v8