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

This reverts commit 99cb4d35.

Reason for revert:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosnap%20-%20debug/builds/16445

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}

TBR=jgruber@chromium.org,bmeurer@chromium.org

Change-Id: I2900b5f087e78f1d321724f03bd063a5ff094183
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:5269, v8:6936, v8:7109, v8:7137
Reviewed-on: https://chromium-review.googlesource.com/796150Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49703}

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(string, start, end);
+  Node* const first = SubString(context, 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(string, start, end);
+    Node* const capture = SubString(context, 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(string, match_from, match_to);
+        Node* match = SubString(context, 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(string, from, to);
+      Node* const substr = SubString(context, 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(string, from, to);
+          Node* const substr = SubString(context, 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(string, from, to);
+    Node* const substr = SubString(context, 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(string, var_last_match_end.value(), match_start);
+            SubString(context, 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(string, var_last_match_end.value(), match_start);
+            SubString(context, 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(string, var_last_match_end.value(), string_length);
+        SubString(context, 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,7 +1773,8 @@ TF_BUILTIN(StringPrototypeSlice, StringBuiltinsAssembler) {
     GotoIf(SmiLessThanOrEqual(var_end.value(), var_start.value()),
            &return_emptystring);
     Node* const result =
-        SubString(subject_string, var_start.value(), var_end.value());
+        SubString(context, subject_string, var_start.value(), var_end.value(),
+                  SubStringFlags::FROM_TO_ARE_BOUNDED);
     args.PopAndReturn(result);
   }
 
@@ -1971,7 +1972,7 @@ TF_BUILTIN(StringPrototypeSubstr, StringBuiltinsAssembler) {
   BIND(&out);
   {
     TNode<Smi> const end = SmiAdd(var_start, var_result_length);
-    Node* const result = SubString(string, var_start, end);
+    Node* const result = SubString(context, string, var_start, end);
     args.PopAndReturn(result);
   }
 }
@@ -2026,11 +2027,12 @@ 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(string, from, to));
+  Return(SubString(context, string, from, to));
 }
 
 // ES6 #sec-string.prototype.substring
@@ -2083,7 +2085,8 @@ TF_BUILTIN(StringPrototypeSubstring, StringBuiltinsAssembler) {
 
   BIND(&out);
   {
-    Node* result = SubString(string, var_start.value(), var_end.value());
+    Node* result =
+        SubString(context, string, var_start.value(), var_end.value());
     args.PopAndReturn(result);
   }
 }
@@ -2137,8 +2140,9 @@ void StringTrimAssembler::Generate(String::TrimMode mode,
         IntPtrConstant(-1), -1, &return_emptystring);
   }
 
-  arguments.PopAndReturn(SubString(string, SmiTag(var_start),
-                                   SmiAdd(SmiTag(var_end), SmiConstant(1))));
+  arguments.PopAndReturn(SubString(context, string, SmiTag(var_start),
+                                   SmiAdd(SmiTag(var_end), SmiConstant(1)),
+                                   SubStringFlags::FROM_TO_ARE_BOUNDED));
 
   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* from, Node* from_instance_type, TNode<IntPtrT> from_index,
-    TNode<Smi> character_count) {
+    Node* context, 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,8 +4647,7 @@ Node* CodeStubAssembler::AllocAndCopyStringCharacters(
   // The subject string is a sequential one-byte string.
   BIND(&one_byte_sequential);
   {
-    Node* result =
-        AllocateSeqOneByteString(NoContextConstant(), character_count);
+    Node* result = AllocateSeqOneByteString(context, character_count);
     CopyStringCharacters(from, result, from_index, IntPtrConstant(0),
                          SmiUntag(character_count), String::ONE_BYTE_ENCODING,
                          String::ONE_BYTE_ENCODING);
@@ -4660,8 +4659,7 @@ Node* CodeStubAssembler::AllocAndCopyStringCharacters(
   // The subject string is a sequential two-byte string.
   BIND(&two_byte_sequential);
   {
-    Node* result =
-        AllocateSeqTwoByteString(NoContextConstant(), character_count);
+    Node* result = AllocateSeqTwoByteString(context, character_count);
     CopyStringCharacters(from, result, from_index, IntPtrConstant(0),
                          SmiUntag(character_count), String::TWO_BYTE_ENCODING,
                          String::TWO_BYTE_ENCODING);
@@ -4674,7 +4672,11 @@ Node* CodeStubAssembler::AllocAndCopyStringCharacters(
   return var_result.value();
 }
 
-Node* CodeStubAssembler::SubString(Node* string, Node* from, Node* to) {
+
+Node* CodeStubAssembler::SubString(Node* context, Node* string, Node* from,
+                                   Node* to, SubStringFlags flags) {
+  DCHECK(flags == SubStringFlags::NONE ||
+         flags == SubStringFlags::FROM_TO_ARE_BOUNDED);
   VARIABLE(var_result, MachineRepresentation::kTagged);
   ToDirectStringAssembler to_direct(state(), string);
   Label end(this), runtime(this);
@@ -4684,8 +4686,14 @@ Node* CodeStubAssembler::SubString(Node* string, Node* from, Node* to) {
   CSA_ASSERT(this, IsString(string));
 
   // Make sure that both from and to are non-negative smis.
-  CSA_ASSERT(this, TaggedIsPositiveSmi(from));
-  CSA_ASSERT(this, TaggedIsPositiveSmi(to));
+
+  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);
@@ -4751,8 +4759,9 @@ Node* CodeStubAssembler::SubString(Node* string, Node* from, Node* to) {
     // encoding at this point.
     GotoIf(to_direct.is_external(), &external_string);
 
-    var_result.Bind(AllocAndCopyStringCharacters(
-        direct_string, instance_type, SmiUntag(offset), substr_length));
+    var_result.Bind(
+        AllocAndCopyStringCharacters(context, direct_string, instance_type,
+                                     SmiUntag(offset), substr_length));
 
     Counters* counters = isolate()->counters();
     IncrementCounter(counters->sub_string_native(), 1);
@@ -4765,9 +4774,9 @@ Node* CodeStubAssembler::SubString(Node* string, Node* from, Node* to) {
   {
     Node* const fake_sequential_string = to_direct.PointerToString(&runtime);
 
-    var_result.Bind(
-        AllocAndCopyStringCharacters(fake_sequential_string, instance_type,
-                                     SmiUntag(offset), substr_length));
+    var_result.Bind(AllocAndCopyStringCharacters(
+        context, fake_sequential_string, instance_type, SmiUntag(offset),
+        substr_length));
 
     Counters* counters = isolate()->counters();
     IncrementCounter(counters->sub_string_native(), 1);
@@ -4785,7 +4794,14 @@ Node* CodeStubAssembler::SubString(Node* string, Node* from, Node* to) {
 
   BIND(&original_string_or_invalid_length);
   {
-    CSA_ASSERT(this, SmiEqual(substr_length, string_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);
@@ -4802,8 +4818,8 @@ Node* CodeStubAssembler::SubString(Node* string, Node* from, Node* to) {
   // Fall back to a runtime call.
   BIND(&runtime);
   {
-    var_result.Bind(CallRuntime(Runtime::kSubString, NoContextConstant(),
-                                string, from, to));
+    var_result.Bind(
+        CallRuntime(Runtime::kSubString, context, string, from, to));
     Goto(&end);
   }
 

src/code-stub-assembler.h

@@ -1150,9 +1150,14 @@ 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.
-  Node* SubString(Node* string, Node* from, Node* to);
+  // 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);
 
   // Return a new string object produced by concatenating |first| with |second|.
   Node* StringAdd(Node* context, Node* first, Node* second,
@@ -1915,7 +1920,8 @@ 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* from, Node* from_instance_type,
+  Node* AllocAndCopyStringCharacters(Node* context, Node* from,
+                                     Node* from_instance_type,
                                      TNode<IntPtrT> from_index,
                                      TNode<Smi> character_count);
 

src/runtime/runtime-strings.cc

@@ -219,13 +219,32 @@ RUNTIME_FUNCTION(Runtime_StringLastIndexOf) {
 RUNTIME_FUNCTION(Runtime_SubString) {
   HandleScope scope(isolate);
   DCHECK_EQ(3, args.length());
+
   CONVERT_ARG_HANDLE_CHECKED(String, string, 0);
-  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());
+  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();
+  }
   isolate->counters()->sub_string_runtime()->Increment();
+
   return *isolate->factory()->NewSubString(string, start, end);
 }
 

test/cctest/test-strings.cc

@@ -1335,6 +1335,99 @@ 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;