[strings] Serialize SeqString padding as 0s without mutation

SeqStrings have their padding bytes serialized as 0s for deterministic
snapshot contents. Currently this is done by mutating the SeqStrings and
memsetting their padding bytes to 0 when serializing. This mutation is
not threadsafe in the presence of shared strings. This CL removes the
mutation by serializing the data and padding payloads separately for
SeqStrings.

Bug: v8:12939
Change-Id: I58c3ada767ce41e0a874a2d6e6392a86142fa1e1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3715715Reviewed-by: Patrick Thier <pthier@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81334}

src/execution/isolate.cc

@@ -4209,7 +4209,6 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
     CodePageCollectionMemoryModificationScope modification_scope(heap());
 
     if (create_heap_objects) {
-      heap_.read_only_space()->ClearStringPaddingIfNeeded();
       read_only_heap_->OnCreateHeapObjectsComplete(this);
     } else {
       SharedHeapDeserializer shared_heap_deserializer(

src/heap/read-only-spaces.cc

@@ -290,7 +290,6 @@ ReadOnlySpace::ReadOnlySpace(Heap* heap)
     : BaseSpace(heap, RO_SPACE),
       top_(kNullAddress),
       limit_(kNullAddress),
-      is_string_padding_cleared_(heap->isolate()->initialized_from_snapshot()),
       capacity_(0),
       area_size_(MemoryChunkLayout::AllocatableMemoryInMemoryChunk(RO_SPACE)) {}
 
@@ -380,24 +379,6 @@ void ReadOnlySpace::RepairFreeSpacesAfterDeserialization() {
   }
 }
 
-void ReadOnlySpace::ClearStringPaddingIfNeeded() {
-  if (V8_ENABLE_THIRD_PARTY_HEAP_BOOL) {
-    // TODO(v8:11641): Revisit this once third-party heap supports iteration.
-    return;
-  }
-  if (is_string_padding_cleared_) return;
-
-  ReadOnlyHeapObjectIterator iterator(this);
-  for (HeapObject o = iterator.Next(); !o.is_null(); 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::Seal(SealMode ro_mode) {
   DCHECK(!is_marked_read_only_);
 

src/heap/read-only-spaces.h

@@ -265,11 +265,6 @@ class ReadOnlySpace : public BaseSpace {
   void EnsureSpaceForAllocation(int size_in_bytes);
   void FreeLinearAllocationArea();
 
-  // 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_;
-
   size_t capacity_;
   const size_t area_size_;
 };

src/objects/string.cc

@@ -1643,16 +1643,25 @@ Handle<String> SeqString::Truncate(Handle<SeqString> string, int new_length) {
   return string;
 }
 
-void SeqOneByteString::clear_padding() {
+SeqString::DataAndPaddingSizes SeqString::GetDataAndPaddingSizes() const {
+  if (IsSeqOneByteString()) {
+    return SeqOneByteString::cast(*this).GetDataAndPaddingSizes();
+  }
+  return SeqTwoByteString::cast(*this).GetDataAndPaddingSizes();
+}
+
+SeqString::DataAndPaddingSizes SeqOneByteString::GetDataAndPaddingSizes()
+    const {
   int data_size = SeqString::kHeaderSize + length() * kOneByteSize;
-  memset(reinterpret_cast<void*>(address() + data_size), 0,
-         SizeFor(length()) - data_size);
+  int padding_size = SizeFor(length()) - data_size;
+  return DataAndPaddingSizes{data_size, padding_size};
 }
 
-void SeqTwoByteString::clear_padding() {
+SeqString::DataAndPaddingSizes SeqTwoByteString::GetDataAndPaddingSizes()
+    const {
   int data_size = SeqString::kHeaderSize + length() * base::kUC16Size;
-  memset(reinterpret_cast<void*>(address() + data_size), 0,
-         SizeFor(length()) - data_size);
+  int padding_size = SizeFor(length()) - data_size;
+  return DataAndPaddingSizes{data_size, padding_size};
 }
 
 uint16_t ConsString::Get(

src/objects/string.h

@@ -694,6 +694,12 @@ class SeqString : public TorqueGeneratedSeqString<SeqString, String> {
   V8_WARN_UNUSED_RESULT static Handle<String> Truncate(Handle<SeqString> string,
                                                        int new_length);
 
+  struct DataAndPaddingSizes {
+    const int data_size;
+    const int padding_size;
+  };
+  DataAndPaddingSizes GetDataAndPaddingSizes() const;
+
   TQ_OBJECT_CONSTRUCTORS(SeqString)
 };
 
@@ -736,9 +742,7 @@ class SeqOneByteString
       const DisallowGarbageCollection& no_gc,
       const SharedStringAccessGuardIfNeeded& access_guard) const;
 
-  // Clear uninitialized padding space. This ensures that the snapshot content
-  // is deterministic.
-  void clear_padding();
+  DataAndPaddingSizes GetDataAndPaddingSizes() const;
 
   // Maximal memory usage for a single sequential one-byte string.
   static const int kMaxCharsSize = kMaxLength;
@@ -782,9 +786,7 @@ class SeqTwoByteString
       const DisallowGarbageCollection& no_gc,
       const SharedStringAccessGuardIfNeeded& access_guard) const;
 
-  // Clear uninitialized padding space. This ensures that the snapshot content
-  // is deterministic.
-  void clear_padding();
+  DataAndPaddingSizes GetDataAndPaddingSizes() const;
 
   // Maximal memory usage for a single sequential two-byte string.
   static const int kMaxCharsSize = kMaxLength * 2;

src/snapshot/serializer.cc

@@ -733,24 +733,16 @@ void Serializer::ObjectSerializer::Serialize() {
   if (InstanceTypeChecker::IsExternalString(instance_type)) {
     SerializeExternalString();
     return;
-  } else if (!ReadOnlyHeap::Contains(*object_)) {
-    // Only clear padding for strings outside the read-only heap. Read-only heap
-    // should have been cleared elsewhere.
-    if (object_->IsSeqOneByteString(cage_base)) {
-      // Clear padding bytes at the end. Done here to avoid having to do this
-      // at allocation sites in generated code.
-      Handle<SeqOneByteString>::cast(object_)->clear_padding();
-    } else if (object_->IsSeqTwoByteString(cage_base)) {
-      Handle<SeqTwoByteString>::cast(object_)->clear_padding();
-    }
   }
   if (InstanceTypeChecker::IsJSTypedArray(instance_type)) {
     SerializeJSTypedArray();
     return;
-  } else if (InstanceTypeChecker::IsJSArrayBuffer(instance_type)) {
+  }
+  if (InstanceTypeChecker::IsJSArrayBuffer(instance_type)) {
     SerializeJSArrayBuffer();
     return;
-  } else if (InstanceTypeChecker::IsScript(instance_type)) {
+  }
+  if (InstanceTypeChecker::IsScript(instance_type)) {
     // Clear cached line ends.
     Oddball undefined = ReadOnlyRoots(isolate()).undefined_value();
     Handle<Script>::cast(object_)->set_line_ends(undefined);
@@ -1218,6 +1210,16 @@ void Serializer::ObjectSerializer::OutputRawData(Address up_to) {
           sink_, object_start, base, bytes_to_output,
           CodeDataContainer::kCodeCageBaseUpper32BitsOffset,
           sizeof(field_value), field_value);
+    } else if (object_->IsSeqString()) {
+      // SeqStrings may contain padding. Serialize the padding bytes as 0s to
+      // make the snapshot content deterministic.
+      SeqString::DataAndPaddingSizes sizes =
+          SeqString::cast(*object_).GetDataAndPaddingSizes();
+      DCHECK_EQ(bytes_to_output, sizes.data_size - base + sizes.padding_size);
+      int data_bytes_to_output = sizes.data_size - base;
+      sink_->PutRaw(reinterpret_cast<byte*>(object_start + base),
+                    data_bytes_to_output, "SeqStringData");
+      sink_->PutN(sizes.padding_size, 0, "SeqStringPadding");
     } else {
       sink_->PutRaw(reinterpret_cast<byte*>(object_start + base),
                     bytes_to_output, "Bytes");

src/snapshot/snapshot-source-sink.cc

@@ -12,6 +12,11 @@
 namespace v8 {
 namespace internal {
 
+void SnapshotByteSink::PutN(int number_of_bytes, const byte v,
+                            const char* description) {
+  data_.insert(data_.end(), number_of_bytes, v);
+}
+
 void SnapshotByteSink::PutInt(uintptr_t integer, const char* description) {
   DCHECK_LT(integer, 1 << 30);
   integer <<= 2;

src/snapshot/snapshot-source-sink.h

@@ -123,6 +123,7 @@ class SnapshotByteSink {
 
   void Put(byte b, const char* description) { data_.push_back(b); }
 
+  void PutN(int number_of_bytes, const byte v, const char* description);
   void PutInt(uintptr_t integer, const char* description);
   void PutRaw(const byte* data, int number_of_bytes, const char* description);