Reland "[cleanup] Harden the SubString CSA/Runtime implementations."

This is a reland of 6d5b54df82e27a82811a836dcdbbfe26829f0e6d
Original change's description:
> [cleanup] Harden the SubString CSA/Runtime implementations.
>
> Remove the self-healing for invalid parameters in the
> CodeStubAssembler::SubString helper and the %SubString runtime function,
> which is used as a fallback for the CodeStubAssembler implementation.
> All call sites must do appropriate parameter validation anyways now that
> the self-hosted JavaScript builtins using these helpers are gone, and we
> have proper contracts with the uses.
>
> Also remove the context parameter from the CodeStubAssembler::SubString
> method, which is unnecessary, since this can no longer throw an
> exception.
>
> Bug: v8:5269, v8:6936, v8:7109, v8:7137
> Change-Id: I19d93bad5f41faa0561c4561a48f78fcba99a549
> Reviewed-on: https://chromium-review.googlesource.com/795720
> Reviewed-by: Jakob Gruber <jgruber@chromium.org>
> Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#49702}

Bug: v8:5269, v8:6936, v8:7109, v8:7137
Change-Id: I5e84998a2dd3990d7981505b401ffc770e0b7ac5
Reviewed-on: https://chromium-review.googlesource.com/913130Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51265}

src/builtins/builtins-regexp-gen.cc

@@ -153,7 +153,7 @@ Node* RegExpBuiltinsAssembler::ConstructNewResultFromMatchInfo(
 
   // Calculate the substring of the first match before creating the result array
   // to avoid an unnecessary write barrier storing the first result.
-  Node* const first = SubString(context, string, start, end);
+  Node* const first = SubString(string, start, end);
 
   Node* const result =
       AllocateRegExpResult(context, num_results, start, string);
@@ -189,7 +189,7 @@ Node* RegExpBuiltinsAssembler::ConstructNewResultFromMatchInfo(
     Node* const from_cursor_plus1 = IntPtrAdd(from_cursor, IntPtrConstant(1));
     Node* const end = LoadFixedArrayElement(match_info, from_cursor_plus1);
 
-    Node* const capture = SubString(context, string, start, end);
+    Node* const capture = SubString(string, start, end);
     StoreFixedArrayElement(result_elements, to_cursor, capture);
     Goto(&next_iter);
 
@@ -1834,7 +1834,7 @@ void RegExpBuiltinsAssembler::RegExpPrototypeMatchBody(Node* const context,
         Node* const match_to = LoadFixedArrayElement(
             match_indices, RegExpMatchInfo::kFirstCaptureIndex + 1);
 
-        Node* match = SubString(context, string, match_from, match_to);
+        Node* match = SubString(string, match_from, match_to);
         var_match.Bind(match);
 
         Goto(&if_didmatch);
@@ -2256,7 +2256,7 @@ void RegExpBuiltinsAssembler::RegExpPrototypeSplitBody(Node* const context,
       Node* const from = last_matched_until;
       Node* const to = match_from;
 
-      TNode<String> const substr = CAST(SubString(context, string, from, to));
+      TNode<String> const substr = CAST(SubString(string, from, to));
       array.Push(substr);
 
       GotoIf(WordEqual(array.length(), int_limit), &out);
@@ -2295,7 +2295,7 @@ void RegExpBuiltinsAssembler::RegExpPrototypeSplitBody(Node* const context,
 
         BIND(&select_capture);
         {
-          Node* const substr = SubString(context, string, from, to);
+          Node* const substr = SubString(string, from, to);
           var_value.Bind(substr);
           Goto(&store_value);
         }
@@ -2333,7 +2333,7 @@ void RegExpBuiltinsAssembler::RegExpPrototypeSplitBody(Node* const context,
     Node* const from = var_last_matched_until.value();
     Node* const to = string_length;
 
-    TNode<String> const substr = CAST(SubString(context, string, from, to));
+    TNode<String> const substr = CAST(SubString(string, from, to));
     array.Push(substr);
 
     Goto(&out);
@@ -2706,7 +2706,7 @@ Node* RegExpBuiltinsAssembler::ReplaceSimpleStringFastPath(
         // TODO(jgruber): We could skip many of the checks that using SubString
         // here entails.
         Node* const first_part =
-            SubString(context, string, var_last_match_end.value(), match_start);
+            SubString(string, var_last_match_end.value(), match_start);
 
         Node* const result = StringAdd(context, var_result.value(), first_part);
         var_result.Bind(result);
@@ -2716,7 +2716,7 @@ Node* RegExpBuiltinsAssembler::ReplaceSimpleStringFastPath(
       BIND(&if_replaceisnotempty);
       {
         Node* const first_part =
-            SubString(context, string, var_last_match_end.value(), match_start);
+            SubString(string, var_last_match_end.value(), match_start);
 
         Node* result = StringAdd(context, var_result.value(), first_part);
         result = StringAdd(context, result, replace_string);
@@ -2745,7 +2745,7 @@ Node* RegExpBuiltinsAssembler::ReplaceSimpleStringFastPath(
   {
     TNode<Smi> const string_length = LoadStringLengthAsSmi(string);
     Node* const last_part =
-        SubString(context, string, var_last_match_end.value(), string_length);
+        SubString(string, var_last_match_end.value(), string_length);
     Node* const result = StringAdd(context, var_result.value(), last_part);
     var_result.Bind(result);
     Goto(&out);

src/builtins/builtins-string-gen.cc

@@ -1689,8 +1689,7 @@ TF_BUILTIN(StringPrototypeSlice, StringBuiltinsAssembler) {
     GotoIf(SmiLessThanOrEqual(var_end.value(), var_start.value()),
            &return_emptystring);
     Node* const result =
-        SubString(context, subject_string, var_start.value(), var_end.value(),
-                  SubStringFlags::FROM_TO_ARE_BOUNDED);
+        SubString(subject_string, var_start.value(), var_end.value());
     args.PopAndReturn(result);
   }
 
@@ -1889,7 +1888,7 @@ TF_BUILTIN(StringPrototypeSubstr, StringBuiltinsAssembler) {
   BIND(&out);
   {
     TNode<Smi> const end = SmiAdd(var_start.value(), var_result_length.value());
-    Node* const result = SubString(context, string, var_start.value(), end);
+    Node* const result = SubString(string, var_start.value(), end);
     args.PopAndReturn(result);
   }
 }
@@ -1944,12 +1943,11 @@ TNode<Smi> StringBuiltinsAssembler::ToSmiBetweenZeroAnd(
 }
 
 TF_BUILTIN(SubString, CodeStubAssembler) {
-  Node* context = Parameter(Descriptor::kContext);
   Node* string = Parameter(Descriptor::kString);
   Node* from = Parameter(Descriptor::kFrom);
   Node* to = Parameter(Descriptor::kTo);
 
-  Return(SubString(context, string, from, to));
+  Return(SubString(string, from, to));
 }
 
 // ES6 #sec-string.prototype.substring
@@ -2002,8 +2000,7 @@ TF_BUILTIN(StringPrototypeSubstring, StringBuiltinsAssembler) {
 
   BIND(&out);
   {
-    Node* result =
-        SubString(context, string, var_start.value(), var_end.value());
+    Node* result = SubString(string, var_start.value(), var_end.value());
     args.PopAndReturn(result);
   }
 }
@@ -2058,9 +2055,8 @@ void StringTrimAssembler::Generate(String::TrimMode mode,
   }
 
   arguments.PopAndReturn(
-      SubString(context, string, SmiTag(var_start.value()),
-                SmiAdd(SmiTag(var_end.value()), SmiConstant(1)),
-                SubStringFlags::FROM_TO_ARE_BOUNDED));
+      SubString(string, SmiTag(var_start.value()),
+                SmiAdd(SmiTag(var_end.value()), SmiConstant(1))));
 
   BIND(&if_runtime);
   arguments.PopAndReturn(

src/code-stub-assembler.cc

@@ -2360,7 +2360,7 @@ Node* CodeStubAssembler::AllocateSeqTwoByteString(int length,
 Node* CodeStubAssembler::AllocateSeqTwoByteString(Node* context,
                                                   TNode<Smi> length,
                                                   AllocationFlags flags) {
-  CSA_SLOW_ASSERT(this, IsFixedArray(context));
+  CSA_SLOW_ASSERT(this, IsZeroOrFixedArray(context));
   Comment("AllocateSeqTwoByteString");
   VARIABLE(var_result, MachineRepresentation::kTagged);
 
@@ -4768,8 +4768,8 @@ TNode<String> CodeStubAssembler::StringFromCharCode(TNode<Int32T> code) {
 // |from_string| must be a sequential string.
 // 0 <= |from_index| <= |from_index| + |character_count| < from_string.length.
 Node* CodeStubAssembler::AllocAndCopyStringCharacters(
-    Node* context, Node* from, Node* from_instance_type,
-    TNode<IntPtrT> from_index, TNode<Smi> character_count) {
+    Node* from, Node* from_instance_type, TNode<IntPtrT> from_index,
+    TNode<Smi> character_count) {
   Label end(this), one_byte_sequential(this), two_byte_sequential(this);
   Variable var_result(this, MachineRepresentation::kTagged);
 
@@ -4779,7 +4779,8 @@ Node* CodeStubAssembler::AllocAndCopyStringCharacters(
   // The subject string is a sequential one-byte string.
   BIND(&one_byte_sequential);
   {
-    Node* result = AllocateSeqOneByteString(context, character_count);
+    Node* result =
+        AllocateSeqOneByteString(NoContextConstant(), character_count);
     CopyStringCharacters(from, result, from_index, IntPtrConstant(0),
                          SmiUntag(character_count), String::ONE_BYTE_ENCODING,
                          String::ONE_BYTE_ENCODING);
@@ -4791,7 +4792,8 @@ Node* CodeStubAssembler::AllocAndCopyStringCharacters(
   // The subject string is a sequential two-byte string.
   BIND(&two_byte_sequential);
   {
-    Node* result = AllocateSeqTwoByteString(context, character_count);
+    Node* result =
+        AllocateSeqTwoByteString(NoContextConstant(), character_count);
     CopyStringCharacters(from, result, from_index, IntPtrConstant(0),
                          SmiUntag(character_count), String::TWO_BYTE_ENCODING,
                          String::TWO_BYTE_ENCODING);
@@ -4804,11 +4806,7 @@ Node* CodeStubAssembler::AllocAndCopyStringCharacters(
   return var_result.value();
 }
 
-
-Node* CodeStubAssembler::SubString(Node* context, Node* string, Node* from,
-                                   Node* to, SubStringFlags flags) {
-  DCHECK(flags == SubStringFlags::NONE ||
-         flags == SubStringFlags::FROM_TO_ARE_BOUNDED);
+Node* CodeStubAssembler::SubString(Node* string, Node* from, Node* to) {
   VARIABLE(var_result, MachineRepresentation::kTagged);
   ToDirectStringAssembler to_direct(state(), string);
   Label end(this), runtime(this);
@@ -4818,14 +4816,8 @@ Node* CodeStubAssembler::SubString(Node* context, Node* string, Node* from,
   CSA_ASSERT(this, IsString(string));
 
   // Make sure that both from and to are non-negative smis.
-
-  if (flags == SubStringFlags::NONE) {
-    GotoIfNot(TaggedIsPositiveSmi(from), &runtime);
-    GotoIfNot(TaggedIsPositiveSmi(to), &runtime);
-  } else {
   CSA_ASSERT(this, TaggedIsPositiveSmi(from));
   CSA_ASSERT(this, TaggedIsPositiveSmi(to));
-  }
 
   TNode<Smi> const substr_length = SmiSub(to, from);
   TNode<Smi> const string_length = LoadStringLengthAsSmi(string);
@@ -4891,9 +4883,8 @@ Node* CodeStubAssembler::SubString(Node* context, Node* string, Node* from,
     // encoding at this point.
     GotoIf(to_direct.is_external(), &external_string);
 
-    var_result.Bind(
-        AllocAndCopyStringCharacters(context, direct_string, instance_type,
-                                     SmiUntag(offset), substr_length));
+    var_result.Bind(AllocAndCopyStringCharacters(
+        direct_string, instance_type, SmiUntag(offset), substr_length));
 
     Counters* counters = isolate()->counters();
     IncrementCounter(counters->sub_string_native(), 1);
@@ -4906,9 +4897,9 @@ Node* CodeStubAssembler::SubString(Node* context, Node* string, Node* from,
   {
     Node* const fake_sequential_string = to_direct.PointerToString(&runtime);
 
-    var_result.Bind(AllocAndCopyStringCharacters(
-        context, fake_sequential_string, instance_type, SmiUntag(offset),
-        substr_length));
+    var_result.Bind(
+        AllocAndCopyStringCharacters(fake_sequential_string, instance_type,
+                                     SmiUntag(offset), substr_length));
 
     Counters* counters = isolate()->counters();
     IncrementCounter(counters->sub_string_native(), 1);
@@ -4926,14 +4917,7 @@ Node* CodeStubAssembler::SubString(Node* context, Node* string, Node* from,
 
   BIND(&original_string_or_invalid_length);
   {
-    if (flags == SubStringFlags::NONE) {
-      // Longer than original string's length or negative: unsafe arguments.
-      GotoIf(SmiAbove(substr_length, string_length), &runtime);
-    } else {
-      // with flag SubStringFlags::FROM_TO_ARE_BOUNDED, the only way we can
-      // get here is if substr_length is equal to string_length.
     CSA_ASSERT(this, SmiEqual(substr_length, string_length));
-    }
 
     // Equal length - check if {from, to} == {0, str.length}.
     GotoIf(SmiAbove(from, SmiConstant(0)), &runtime);
@@ -4950,8 +4934,8 @@ Node* CodeStubAssembler::SubString(Node* context, Node* string, Node* from,
   // Fall back to a runtime call.
   BIND(&runtime);
   {
-    var_result.Bind(
-        CallRuntime(Runtime::kSubString, context, string, from, to));
+    var_result.Bind(CallRuntime(Runtime::kSubString, NoContextConstant(),
+                                string, from, to));
     Goto(&end);
   }
 

src/code-stub-assembler.h

@@ -1197,14 +1197,9 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
   // Return the single character string with only {code}.
   TNode<String> StringFromCharCode(TNode<Int32T> code);
 
-  enum class SubStringFlags { NONE, FROM_TO_ARE_BOUNDED };
-
   // Return a new string object which holds a substring containing the range
   // [from,to[ of string.  |from| and |to| are expected to be tagged.
-  // If flags has the value FROM_TO_ARE_BOUNDED then from and to are in
-  // the range [0, string-length)
-  Node* SubString(Node* context, Node* string, Node* from, Node* to,
-                  SubStringFlags flags = SubStringFlags::NONE);
+  Node* SubString(Node* string, Node* from, Node* to);
 
   // Return a new string object produced by concatenating |first| with |second|.
   Node* StringAdd(Node* context, Node* first, Node* second,
@@ -2006,8 +2001,7 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
   Node* CollectFeedbackForString(Node* instance_type);
   void GenerateEqual_Same(Node* value, Label* if_equal, Label* if_notequal,
                           Variable* var_type_feedback = nullptr);
-  Node* AllocAndCopyStringCharacters(Node* context, Node* from,
-                                     Node* from_instance_type,
+  Node* AllocAndCopyStringCharacters(Node* from, Node* from_instance_type,
                                      TNode<IntPtrT> from_index,
                                      TNode<Smi> character_count);
 

src/runtime/runtime-strings.cc

@@ -219,32 +219,13 @@ RUNTIME_FUNCTION(Runtime_StringLastIndexOf) {
 RUNTIME_FUNCTION(Runtime_SubString) {
   HandleScope scope(isolate);
   DCHECK_EQ(3, args.length());
-
   CONVERT_ARG_HANDLE_CHECKED(String, string, 0);
-  int start, end;
-  // We have a fast integer-only case here to avoid a conversion to double in
-  // the common case where from and to are Smis.
-  if (args[1]->IsSmi() && args[2]->IsSmi()) {
-    CONVERT_SMI_ARG_CHECKED(from_number, 1);
-    CONVERT_SMI_ARG_CHECKED(to_number, 2);
-    start = from_number;
-    end = to_number;
-  } else if (args[1]->IsNumber() && args[2]->IsNumber()) {
-    CONVERT_DOUBLE_ARG_CHECKED(from_number, 1);
-    CONVERT_DOUBLE_ARG_CHECKED(to_number, 2);
-    start = FastD2IChecked(from_number);
-    end = FastD2IChecked(to_number);
-  } else {
-    return isolate->ThrowIllegalOperation();
-  }
-  // The following condition is intentionally robust because the SubString
-  // builtin delegates here and we test this in
-  // cctest/test-strings/RobustSubStringStub.
-  if (end < start || start < 0 || end > string->length()) {
-    return isolate->ThrowIllegalOperation();
-  }
+  CONVERT_INT32_ARG_CHECKED(start, 1);
+  CONVERT_INT32_ARG_CHECKED(end, 2);
+  DCHECK_LE(0, start);
+  DCHECK_LE(start, end);
+  DCHECK_LE(end, string->length());
   isolate->counters()->sub_string_runtime()->Increment();
-
   return *isolate->factory()->NewSubString(string, start, end);
 }
 

test/cctest/test-strings.cc

@@ -1335,99 +1335,6 @@ UNINITIALIZED_TEST(OneByteArrayJoin) {
   isolate->Dispose();
 }
 
-
-static void CheckException(const char* source) {
-  // An empty handle is returned upon exception.
-  CHECK(CompileRun(source).IsEmpty());
-}
-
-
-TEST(RobustSubStringStub) {
-  // This tests whether the SubStringStub can handle unsafe arguments.
-  // If not recognized, those unsafe arguments lead to out-of-bounds reads.
-  FLAG_allow_natives_syntax = true;
-  CcTest::InitializeVM();
-  v8::HandleScope scope(CcTest::isolate());
-  v8::Local<v8::Value> result;
-  Handle<String> string;
-  CompileRun("var short = 'abcdef';");
-
-  // Invalid indices.
-  CheckException("%_SubString(short,     0,    10000);");
-  CheckException("%_SubString(short, -1234,        5);");
-  CheckException("%_SubString(short,     5,        2);");
-  // Special HeapNumbers.
-  CheckException("%_SubString(short,     1, Infinity);");
-  CheckException("%_SubString(short,   NaN,        5);");
-  // String arguments.
-  CheckException("%_SubString(short,    '2',     '5');");
-  // Ordinary HeapNumbers can be handled (in runtime).
-  result = CompileRun("%_SubString(short, Math.sqrt(4), 5.1);");
-  string = v8::Utils::OpenHandle(v8::String::Cast(*result));
-  CHECK_EQ(0, strcmp("cde", string->ToCString().get()));
-
-  CompileRun("var long = 'abcdefghijklmnopqrstuvwxyz';");
-  // Invalid indices.
-  CheckException("%_SubString(long,     0,    10000);");
-  CheckException("%_SubString(long, -1234,       17);");
-  CheckException("%_SubString(long,    17,        2);");
-  // Special HeapNumbers.
-  CheckException("%_SubString(long,     1, Infinity);");
-  CheckException("%_SubString(long,   NaN,       17);");
-  // String arguments.
-  CheckException("%_SubString(long,    '2',    '17');");
-  // Ordinary HeapNumbers within bounds can be handled (in runtime).
-  result = CompileRun("%_SubString(long, Math.sqrt(4), 17.1);");
-  string = v8::Utils::OpenHandle(v8::String::Cast(*result));
-  CHECK_EQ(0, strcmp("cdefghijklmnopq", string->ToCString().get()));
-
-  // Test that out-of-bounds substring of a slice fails when the indices
-  // would have been valid for the underlying string.
-  CompileRun("var slice = long.slice(1, 15);");
-  CheckException("%_SubString(slice, 0, 17);");
-}
-
-TEST(RobustSubStringStubExternalStrings) {
-  // Ensure that the specific combination of calling the SubStringStub on an
-  // external string and triggering a GC on string allocation does not crash.
-  // See crbug.com/649967.
-
-  FLAG_allow_natives_syntax = true;
-#ifdef VERIFY_HEAP
-  FLAG_verify_heap = true;
-#endif
-
-  CcTest::InitializeVM();
-  v8::HandleScope handle_scope(CcTest::isolate());
-
-  v8::Local<v8::String> underlying =
-      CompileRun(
-          "var str = 'abcdefghijklmnopqrstuvwxyz';"
-          "str")
-          ->ToString(CcTest::isolate()->GetCurrentContext())
-          .ToLocalChecked();
-  CHECK(v8::Utils::OpenHandle(*underlying)->IsSeqOneByteString());
-
-  const int length = underlying->Length();
-  uc16* two_byte = NewArray<uc16>(length + 1);
-  underlying->Write(two_byte);
-
-  Resource* resource = new Resource(two_byte, length);
-  CHECK(underlying->MakeExternal(resource));
-  CHECK(v8::Utils::OpenHandle(*underlying)->IsExternalTwoByteString());
-
-  v8::Local<v8::Script> script = v8_compile(v8_str("%_SubString(str, 5, 8)"));
-
-  // Trigger a GC on string allocation.
-  i::heap::SimulateFullSpace(CcTest::heap()->new_space());
-
-  v8::Local<v8::Value> result;
-  CHECK(script->Run(v8::Isolate::GetCurrent()->GetCurrentContext())
-            .ToLocal(&result));
-  Handle<String> string = v8::Utils::OpenHandle(v8::String::Cast(*result));
-  CHECK_EQ(0, strcmp("fgh", string->ToCString().get()));
-}
-
 namespace {
 
 int* global_use_counts = nullptr;