[objects] Remove MakeExternal case for uncached internal strings

Concurrently accessing internal external uncached strings is not
thread-safe. We are removing a case where we can make such a string
through MakeExternal.

Bug: v8:7790
Change-Id: I958062c15cf40ccc330600bb572de98620866e54
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2565511
Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71573}

src/objects/string.cc

@@ -182,7 +182,9 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
   ReadOnlyRoots roots(isolate);
   if (size < ExternalString::kSizeOfAllExternalStrings) {
     if (is_internalized) {
-      new_map = roots.uncached_external_internalized_string_map();
+      // We do not support this case since accessing internal external
+      // uncached strings is not thread-safe.
+      return false;
     } else {
       new_map = roots.uncached_external_string_map();
     }
@@ -259,9 +261,13 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
   Map new_map;
   ReadOnlyRoots roots(isolate);
   if (size < ExternalString::kSizeOfAllExternalStrings) {
-    new_map = is_internalized
-                  ? roots.uncached_external_one_byte_internalized_string_map()
-                  : roots.uncached_external_one_byte_string_map();
+    if (is_internalized) {
+      // We do not support this case since accessing internal external
+      // uncached strings is not thread-safe.
+      return false;
+    } else {
+      new_map = roots.uncached_external_one_byte_string_map();
+    }
   } else {
     new_map = is_internalized
                   ? roots.external_one_byte_internalized_string_map()
@@ -309,6 +315,11 @@ bool String::SupportsExternalization() {
   DCHECK_LE(ExternalString::kUncachedSize, this->Size());
 #endif
 
+  if (this->Size() < ExternalString::kSizeOfAllExternalStrings &&
+      this->IsInternalizedString()) {
+    return false;
+  }
+
   Isolate* isolate = GetIsolateFromWritableObject(*this);
   return !isolate->heap()->IsInGCPostProcessing();
 }

src/objects/string.h

@@ -360,7 +360,7 @@ class String : public TorqueGeneratedString<String, Name> {
       v8::String::ExternalStringResource* resource);
   V8_EXPORT_PRIVATE bool MakeExternal(
       v8::String::ExternalOneByteStringResource* resource);
-  bool SupportsExternalization();
+  V8_EXPORT_PRIVATE bool SupportsExternalization();
 
   // Conversion.
   // "array index": an index allowed by the ES spec for JSArrays.

test/cctest/heap/test-heap.cc

@@ -5914,7 +5914,7 @@ TEST(Regress631969) {
 
   {
     StaticOneByteResource external_string("12345678901234");
-    s3->MakeExternal(&external_string);
+    CHECK(s3->MakeExternal(&external_string));
     CcTest::CollectGarbage(OLD_SPACE);
     // This avoids the GC from trying to free stack allocated resources.
     i::Handle<i::ExternalOneByteString>::cast(s3)->SetResource(isolate,

test/cctest/test-api.cc

@@ -16873,7 +16873,7 @@ TEST(VisitExternalStrings) {
   v8::Isolate* isolate = CcTest::isolate();
   LocalContext env;
   v8::HandleScope scope(isolate);
-  const char string[] = "Some string";
+  const char string[] = "Some string that's long";
   uint16_t* two_byte_string = AsciiToTwoByteString(string);
   TestResource* resource[4];
   resource[0] = new TestResource(two_byte_string);
@@ -16953,7 +16953,7 @@ TEST(ExternalInternalizedStringCollectedAtTearDown) {
     v8::Local<v8::String> ring =
         CompileRun("ring")->ToString(env.local()).ToLocalChecked();
     CHECK(v8::Utils::OpenHandle(*ring)->IsInternalizedString());
-    ring->MakeExternal(inscription);
+    CHECK(ring->MakeExternal(inscription));
     // Ring is still alive.  Orcs are roaming freely across our lands.
     CHECK_EQ(0, destroyed);
     USE(ring);
@@ -16975,7 +16975,7 @@ TEST(ExternalInternalizedStringCollectedAtGC) {
         new TestOneByteResource(i::StrDup(s), &destroyed);
     v8::Local<v8::String> ring = CompileRun("ring").As<v8::String>();
     CHECK(v8::Utils::OpenHandle(*ring)->IsInternalizedString());
-    ring->MakeExternal(inscription);
+    CHECK(ring->MakeExternal(inscription));
     // Ring is still alive.  Orcs are roaming freely across our lands.
     CHECK_EQ(0, destroyed);
     USE(ring);
@@ -18084,7 +18084,7 @@ THREADED_TEST(TwoByteStringInOneByteCons) {
 
   TestResource resource(uc16_buffer);
 
-  flat_string->MakeExternal(&resource);
+  CHECK(flat_string->MakeExternal(&resource));
 
   CHECK(flat_string->IsTwoByteRepresentation());
 
@@ -21435,7 +21435,7 @@ class RegExpInterruptTest {
     v8::Local<v8::String> string =
         v8::Local<v8::String>::New(isolate, instance->string_handle_);
     CHECK(string->CanMakeExternal());
-    string->MakeExternal(&one_byte_string_resource);
+    CHECK(string->MakeExternal(&one_byte_string_resource));
   }
 
   static void MakeSubjectTwoByteExternal(v8::Isolate* isolate, void* data) {
@@ -21445,7 +21445,7 @@ class RegExpInterruptTest {
     v8::Local<v8::String> string =
         v8::Local<v8::String>::New(isolate, instance->string_handle_);
     CHECK(string->CanMakeExternal());
-    string->MakeExternal(&two_byte_string_resource);
+    CHECK(string->MakeExternal(&two_byte_string_resource));
   }
 
   static void ReenterIrregexp(v8::Isolate* isolate, void* data) {

test/cctest/test-concurrent-string.cc

@@ -19,10 +19,10 @@ namespace internal {
 
 namespace {
 
-#define DOUBLE_VALUE 28.123456789
-#define STRING_VALUE "28.123456789"
+#define DOUBLE_VALUE 12345.123456789
+#define STRING_VALUE "12345.123456789"
 #define ARRAY_VALUE \
-  { '2', '8', '.', '1', '2', '3', '4', '5', '6', '7', '8', '9' }
+  { '1', '2', '3', '4', '5', '.', '1', '2', '3', '4', '5', '6', '7', '8', '9' }
 
 // Adapted from cctest/test-api.cc, and
 // test/cctest/heap/test-external-string-tracker.cc.
@@ -202,7 +202,8 @@ TEST(InspectTwoByteExternalizing) {
   // TODO(solanes): Can we have only one raw string?
   const char* raw_string = STRING_VALUE;
   // TODO(solanes): Is this the best way to create a two byte string from chars?
-  const int kLength = 12;
+  const int kLength = 15;
+  DCHECK_EQ(kLength, strlen(raw_string));
   const uint16_t two_byte_array[kLength] = ARRAY_VALUE;
   Handle<String> two_bytes_string;
   {

test/cctest/test-serialize.cc

@@ -2114,28 +2114,28 @@ TEST(CodeSerializerExternalString) {
   v8::HandleScope scope(CcTest::isolate());
 
   // Obtain external internalized one-byte string.
-  SerializerOneByteResource one_byte_resource("one_byte", 8);
+  SerializerOneByteResource one_byte_resource("one_byte_but_long", 17);
   Handle<String> one_byte_string =
-      isolate->factory()->NewStringFromAsciiChecked("one_byte");
+      isolate->factory()->NewStringFromAsciiChecked("one_byte_but_long");
   one_byte_string = isolate->factory()->InternalizeString(one_byte_string);
   one_byte_string->MakeExternal(&one_byte_resource);
   CHECK(one_byte_string->IsExternalOneByteString());
   CHECK(one_byte_string->IsInternalizedString());
 
   // Obtain external internalized two-byte string.
-  SerializerTwoByteResource two_byte_resource("two_byte", 8);
+  SerializerTwoByteResource two_byte_resource("two_byte_but_long", 17);
   Handle<String> two_byte_string =
-      isolate->factory()->NewStringFromAsciiChecked("two_byte");
+      isolate->factory()->NewStringFromAsciiChecked("two_byte_but_long");
   two_byte_string = isolate->factory()->InternalizeString(two_byte_string);
   two_byte_string->MakeExternal(&two_byte_resource);
   CHECK(two_byte_string->IsExternalTwoByteString());
   CHECK(two_byte_string->IsInternalizedString());
 
   const char* source =
-      "var o = {}               \n"
-      "o.one_byte = 7;          \n"
-      "o.two_byte = 8;          \n"
-      "o.one_byte + o.two_byte; \n";
+      "var o = {}                                 \n"
+      "o.one_byte_but_long = 7;                   \n"
+      "o.two_byte_but_long = 8;                   \n"
+      "o.one_byte_but_long + o.two_byte_but_long; \n";
   Handle<String> source_string = isolate->factory()
                                      ->NewStringFromUtf8(CStrVector(source))
                                      .ToHandleChecked();

test/cctest/test-strings.cc

@@ -39,6 +39,7 @@
 #include "src/heap/heap-inl.h"
 #include "src/init/v8.h"
 #include "src/objects/objects-inl.h"
+#include "src/objects/string.h"
 #include "src/strings/unicode-decoder.h"
 #include "test/cctest/cctest.h"
 #include "test/cctest/heap/heap-utils.h"
@@ -1901,20 +1902,6 @@ TEST(StringEquals) {
   CHECK(!bar_str->StringEquals(foo_str2));
 }
 
-class OneByteStringResource : public v8::String::ExternalOneByteStringResource {
- public:
-  // Takes ownership of |data|.
-  OneByteStringResource(char* data, size_t length)
-      : data_(data), length_(length) {}
-  ~OneByteStringResource() override { delete[] data_; }
-  const char* data() const override { return data_; }
-  size_t length() const override { return length_; }
-
- private:
-  char* data_;
-  size_t length_;
-};
-
 TEST(Regress876759) {
   // Thin strings are used in conjunction with young gen
   if (FLAG_single_generation) return;
@@ -1949,9 +1936,9 @@ TEST(Regress876759) {
   Handle<String> grandparent =
       handle(ThinString::cast(*parent).actual(), isolate);
   CHECK_EQ(*parent, SlicedString::cast(*sliced).parent());
-  OneByteStringResource* resource =
-      new OneByteStringResource(external_one_byte_buf, kLength);
-  grandparent->MakeExternal(resource);
+  OneByteResource* resource =
+      new OneByteResource(external_one_byte_buf, kLength);
+  CHECK(grandparent->MakeExternal(resource));
   // The grandparent string becomes one-byte, but the child strings are still
   // two-byte.
   CHECK(grandparent->IsOneByteRepresentation());
@@ -1961,6 +1948,69 @@ TEST(Regress876759) {
   CHECK(String::IsOneByteRepresentationUnderneath(*sliced));
 }
 
+// Show failure when trying to create and internal, external and uncached string
+// through MakeExternal. One byte version.
+TEST(MakeExternalCreationFailureOneByte) {
+  Isolate* isolate = CcTest::i_isolate();
+  Factory* factory = isolate->factory();
+// Due to different size restrictions the string needs to be small but not too
+// small. One of these restrictions is whether pointer compression is enabled.
+#ifdef V8_COMPRESS_POINTERS
+  const char* raw_small = "small string";
+#elif V8_TARGET_ARCH_32_BIT
+  const char* raw_small = "smol";
+#else
+  const char* raw_small = "smalls";
+#endif  // V8_COMPRESS_POINTERS
+
+  HandleScope handle_scope(isolate);
+  Handle<String> one_byte_string =
+      factory->InternalizeString(factory->NewStringFromAsciiChecked(raw_small));
+  CHECK(one_byte_string->IsOneByteRepresentation());
+  CHECK(!one_byte_string->IsExternalString());
+  CHECK(!one_byte_string->SupportsExternalization());
+}
+
+// Show failure when trying to create and internal, external and uncached string
+// through MakeExternal. Two byte version.
+TEST(MakeExternalCreationFailureTwoByte) {
+  Isolate* isolate = CcTest::i_isolate();
+  Factory* factory = isolate->factory();
+  // Due to different size restrictions the string needs to be small but not too
+  // small.
+  const char* raw_small = "smalls";
+  const int kLength = 6;
+  DCHECK_EQ(kLength, strlen(raw_small));
+  const uint16_t two_byte_array[kLength] = {'s', 'm', 'a', 'l', 'l', 's'};
+
+  HandleScope handle_scope(isolate);
+  Handle<String> two_bytes_string;
+  {
+    Handle<SeqTwoByteString> raw =
+        factory->NewRawTwoByteString(kLength).ToHandleChecked();
+    DisallowGarbageCollection no_gc;
+    CopyChars(raw->GetChars(no_gc), two_byte_array, kLength);
+    two_bytes_string = raw;
+  }
+  two_bytes_string = factory->InternalizeString(two_bytes_string);
+  CHECK(two_bytes_string->IsTwoByteRepresentation());
+  CHECK(!two_bytes_string->IsExternalString());
+  if (COMPRESS_POINTERS_BOOL) {
+    CHECK(!two_bytes_string->SupportsExternalization());
+  } else {
+    // Without pointer compression, there is no string size that can cause a
+    // failure for a two byte string. It needs to be bigger than 5 chars to
+    // support externalization, but at that point is bigger than the limit and
+    // it is not uncached anymore.
+    // As a note, since pointer compression is only enabled for 64 bits, all
+    // target 32 bit archs fall in this case.
+    CHECK(two_bytes_string->MakeExternal(
+        new Resource(AsciiToTwoByteString(raw_small), strlen(raw_small))));
+    auto external = Handle<ExternalString>::cast(two_bytes_string);
+    CHECK(!external->is_uncached());
+  }
+}
+
 }  // namespace test_strings
 }  // namespace internal
 }  // namespace v8

test/mjsunit/json2.js

@@ -180,11 +180,11 @@ Object.defineProperty(non_enum, "b", { value: 2, enumerable: false });
 non_enum.c = 3;
 TestStringify('{"a":1,"c":3}', non_enum);
 
-var str = "external";
+var str = "external_string";
 try {
   externalizeString(str, true);
 } catch (e) { }
-TestStringify("\"external\"", str, null, 0);
+TestStringify("\"external_string\"", str, null, 0);
 
 var o = {};
 o.somespecialproperty = 10;

test/mjsunit/string-external-cached.js

@@ -53,7 +53,7 @@ function test() {
     assertEquals('B', charat_str[i].charAt(3*16 + 11));
   }
 
-  charat_short = "01234";
+  charat_short = "0123456789ABC";
   try {  // String can only be externalized once
     externalizeString(charat_short, true);
   } catch (ex) { }