Reland "[heap] Mark RO_SPACE as read-only after deserialization"

This is a reland of 40f1aaf3

Put back padding clearing into the SerializeObject method but only when
the String is not in RO_SPACE. For RO_SPACE strings, if required
iterate over the space before serialization clearing the strings.

Original change's description:
> [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}

Bug: v8:7464
Change-Id: Ia8386c4ff5f5df3207f584caf7a9b1ff1e405f25
Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/1042145Reviewed-by: Hannes Payer <hpayer@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Dan Elphick <delphick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53087}

src/api.cc

@@ -737,6 +737,8 @@ StartupData SnapshotCreator::CreateBlob(
       i::GarbageCollectionReason::kSnapshotCreator);
   isolate->heap()->CompactFixedArraysOfWeakCells();
 
+  isolate->heap()->read_only_space()->ClearStringPaddingIfNeeded();
+
   i::DisallowHeapAllocation no_gc_from_here_on;
 
   int num_contexts = num_additional_contexts + 1;
@@ -6765,8 +6767,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

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

src/heap/setup-heap-internal.cc

@@ -555,6 +555,9 @@ bool Heap::CreateInitialMaps() {
 #undef ALLOCATE_EMPTY_FIXED_TYPED_ARRAY
 
   DCHECK(!InNewSpace(empty_fixed_array()));
+
+  bigint_map()->SetConstructorFunctionIndex(Context::BIGINT_FUNCTION_INDEX);
+
   return true;
 }
 
@@ -849,6 +852,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

@@ -3195,6 +3195,51 @@ bool PagedSpace::RawSlowRefillLinearAllocationArea(int size_in_bytes) {
 void MapSpace::VerifyObject(HeapObject* object) { CHECK(object->IsMap()); }
 #endif
 
+ReadOnlySpace::ReadOnlySpace(Heap* heap, AllocationSpace id,
+                             Executability executable)
+    : PagedSpace(heap, id, executable),
+      is_string_padding_cleared_(heap->isolate()->initialized_from_snapshot()) {
+}
+
+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::ClearStringPaddingIfNeeded() {
+  if (is_string_padding_cleared_) return;
+
+  WritableScope writable_scope(this);
+  for (Page* page : *this) {
+    HeapObjectIterator iterator(page);
+    for (HeapObject* o = iterator.Next(); o != nullptr; o = iterator.Next()) {
+      if (o->IsSeqOneByteString()) {
+        SeqOneByteString::cast(o)->clear_padding();
+      } else if (o->IsSeqTwoByteString()) {
+        SeqTwoByteString::cast(o)->clear_padding();
+      }
+    }
+  }
+  is_string_padding_cleared_ = true;
+}
+
+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

@@ -2902,8 +2902,38 @@ class MapSpace : public PagedSpace {
 
 class ReadOnlySpace : public PagedSpace {
  public:
-  ReadOnlySpace(Heap* heap, AllocationSpace id, Executability executable)
-      : PagedSpace(heap, id, executable) {}
+  class WritableScope {
+   public:
+    explicit WritableScope(ReadOnlySpace* space) : space_(space) {
+      space_->MarkAsReadWrite();
+    }
+
+    ~WritableScope() { space_->MarkAsReadOnly(); }
+
+   private:
+    ReadOnlySpace* space_;
+  };
+
+  ReadOnlySpace(Heap* heap, AllocationSpace id, Executability executable);
+
+  ~ReadOnlySpace() {
+    // Mark as writable as tearing down the space writes to it.
+    // MarkAsReadWrite();
+  }
+
+  void ClearStringPaddingIfNeeded();
+  void MarkAsReadOnly();
+
+ private:
+  void MarkAsReadWrite();
+  void SetPermissionsForPages(PageAllocator::Permission access);
+
+  bool is_marked_read_only_ = false;
+  //
+  // String padding must be cleared just before serialization and therefore the
+  // string padding in the space will already have been cleared if the space was
+  // deserialized.
+  bool is_string_padding_cleared_;
 };
 
 // -----------------------------------------------------------------------------

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/code-serializer.cc

@@ -54,6 +54,8 @@ ScriptCompiler::CachedData* CodeSerializer::Serialize(
   if (script->ContainsAsmModule()) return nullptr;
   if (isolate->debug()->is_loaded()) return nullptr;
 
+  isolate->heap()->read_only_space()->ClearStringPaddingIfNeeded();
+
   // Serialize code object.
   Handle<String> source(String::cast(script->source()), isolate);
   CodeSerializer cs(isolate, SerializedCodeData::SourceHash(source));

src/snapshot/serializer.cc

@@ -609,12 +609,16 @@ 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();
+  } else if (!serializer_->isolate()->heap()->InReadOnlySpace(object_)) {
+    // Only clear padding for strings outside RO_SPACE. RO_SPACE should have
+    // been cleared elsewhere.
+    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-serialize.cc

@@ -458,6 +458,7 @@ static void PartiallySerializeContext(Vector<const byte>* startup_blob_out,
 
     env.Reset();
 
+    isolate->heap()->read_only_space()->ClearStringPaddingIfNeeded();
     SnapshotByteSink startup_sink;
     StartupSerializer startup_serializer(isolate);
     startup_serializer.SerializeStrongReferences();
@@ -582,6 +583,7 @@ static void PartiallySerializeCustomContext(
 
     env.Reset();
 
+    isolate->heap()->read_only_space()->ClearStringPaddingIfNeeded();
     SnapshotByteSink startup_sink;
     StartupSerializer startup_serializer(isolate);
     startup_serializer.SerializeStrongReferences();

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