[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/795720Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49702}

src/builtins/builtins-regexp-gen.cc

@@ -152,7 +152,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);
@@ -188,7 +188,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);
 
@@ -2008,7 +2008,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);
@@ -2430,7 +2430,7 @@ void RegExpBuiltinsAssembler::RegExpPrototypeSplitBody(Node* const context,
       Node* const from = last_matched_until;
       Node* const to = match_from;
 
-      Node* const substr = SubString(context, string, from, to);
+      Node* const substr = SubString(string, from, to);
       array.Push(substr);
 
       GotoIf(WordEqual(array.length(), int_limit), &out);
@@ -2469,7 +2469,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);
         }
@@ -2507,7 +2507,7 @@ void RegExpBuiltinsAssembler::RegExpPrototypeSplitBody(Node* const context,
     Node* const from = var_last_matched_until.value();
     Node* const to = string_length;
 
-    Node* const substr = SubString(context, string, from, to);
+    Node* const substr = SubString(string, from, to);
     array.Push(substr);
 
     Goto(&out);
@@ -2893,7 +2893,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);
@@ -2903,7 +2903,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);
@@ -2932,7 +2932,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

@@ -1773,8 +1773,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);
   }
 
@@ -1972,7 +1971,7 @@ TF_BUILTIN(StringPrototypeSubstr, StringBuiltinsAssembler) {
   BIND(&out);
   {
     TNode<Smi> const end = SmiAdd(var_start, var_result_length);
-    Node* const result = SubString(context, string, var_start, end);
+    Node* const result = SubString(string, var_start, end);
     args.PopAndReturn(result);
   }
 }
@@ -2027,12 +2026,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
@@ -2085,8 +2083,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);
   }
 }
@@ -2140,9 +2137,8 @@ void StringTrimAssembler::Generate(String::TrimMode mode,
         IntPtrConstant(-1), -1, &return_emptystring);
   }
 
-  arguments.PopAndReturn(SubString(context, string, SmiTag(var_start),
-                                   SmiAdd(SmiTag(var_end), SmiConstant(1)),
-                                   SubStringFlags::FROM_TO_ARE_BOUNDED));
+  arguments.PopAndReturn(SubString(string, SmiTag(var_start),
+                                   SmiAdd(SmiTag(var_end), SmiConstant(1))));
 
   BIND(&if_runtime);
   arguments.PopAndReturn(

src/code-stub-assembler.cc

@@ -4636,8 +4636,8 @@ Node* CodeStubAssembler::StringFromCharCode(Node* 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);
 
@@ -4647,7 +4647,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);
@@ -4659,7 +4660,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);
@@ -4672,11 +4674,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);
@@ -4686,14 +4684,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));
-  }
+  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);
@@ -4759,9 +4751,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);
@@ -4774,9 +4765,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);
@@ -4794,14 +4785,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));
-    }
+    CSA_ASSERT(this, SmiEqual(substr_length, string_length));
 
     // Equal length - check if {from, to} == {0, str.length}.
     GotoIf(SmiAbove(from, SmiConstant(0)), &runtime);
@@ -4818,8 +4802,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

@@ -1150,14 +1150,9 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
   // Return the single character string with only {code}.
   Node* StringFromCharCode(Node* 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,
@@ -1920,8 +1915,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;