From b4c11d0816897d8bef0e65050749bf91f46ffc9e Mon Sep 17 00:00:00 2001 From: "sgjesse@chromium.org" Date: Mon, 2 Nov 2009 12:21:43 +0000 Subject: [PATCH] Don't use string slices when processing RexExp replace (re-apply r3153) Re-apply r3153 with a fix for issue 490. Except for the change in line 1756 and the added test this change is identical to http://codereview.chromium.org/342015. BUG=490 TEST=test/mjsunit/regress/regress-490.js Review URL: http://codereview.chromium.org/341064 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@3197 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/runtime.cc | 56 +++++++++++++++++++++++------ src/string.js | 9 +++-- test/mjsunit/regress/regress-490.js | 48 +++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 14 deletions(-) create mode 100644 test/mjsunit/regress/regress-490.js diff --git a/src/runtime.cc b/src/runtime.cc index 8fd62c986c..2596ae0eee 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -1357,8 +1357,9 @@ class ReplacementStringBuilder { StringBuilderSubstringPosition::encode(from); AddElement(Smi::FromInt(encoded_slice)); } else { - Handle slice = Factory::NewStringSlice(subject_, from, to); - AddElement(*slice); + // Otherwise encode as two smis. + AddElement(Smi::FromInt(-length)); + AddElement(Smi::FromInt(from)); } IncrementCharacterCount(length); } @@ -1750,8 +1751,9 @@ static Object* StringReplaceRegExpWithString(String* subject, int prev = 0; // Number of parts added by compiled replacement plus preceeding string - // and possibly suffix after last match. - const int parts_added_per_loop = compiled_replacement.parts() + 2; + // and possibly suffix after last match. It is possible for compiled + // replacements to use two elements when encoded as two smis. + const int parts_added_per_loop = compiled_replacement.parts() * 2 + 2; bool matched = true; do { ASSERT(last_match_info_handle->HasFastElements()); @@ -3766,9 +3768,21 @@ static inline void StringBuilderConcatHelper(String* special, for (int i = 0; i < array_length; i++) { Object* element = fixed_array->get(i); if (element->IsSmi()) { + // Smi encoding of position and length. int encoded_slice = Smi::cast(element)->value(); - int pos = StringBuilderSubstringPosition::decode(encoded_slice); - int len = StringBuilderSubstringLength::decode(encoded_slice); + int pos; + int len; + if (encoded_slice > 0) { + // Position and length encoded in one smi. + pos = StringBuilderSubstringPosition::decode(encoded_slice); + len = StringBuilderSubstringLength::decode(encoded_slice); + } else { + // Position and length encoded in two smis. + Object* obj = fixed_array->get(++i); + ASSERT(obj->IsSmi()); + pos = Smi::cast(obj)->value(); + len = -encoded_slice; + } String::WriteToFlat(special, sink + position, pos, @@ -3789,6 +3803,10 @@ static Object* Runtime_StringBuilderConcat(Arguments args) { ASSERT(args.length() == 2); CONVERT_CHECKED(JSArray, array, args[0]); CONVERT_CHECKED(String, special, args[1]); + + // This assumption is used by the slice encoding in one or two smis. + ASSERT(Smi::kMaxValue >= String::kMaxLength); + int special_length = special->length(); Object* smi_array_length = array->length(); if (!smi_array_length->IsSmi()) { @@ -3816,13 +3834,29 @@ static Object* Runtime_StringBuilderConcat(Arguments args) { for (int i = 0; i < array_length; i++) { Object* elt = fixed_array->get(i); if (elt->IsSmi()) { + // Smi encoding of position and length. int len = Smi::cast(elt)->value(); - int pos = len >> 11; - len &= 0x7ff; - if (pos + len > special_length) { - return Top::Throw(Heap::illegal_argument_symbol()); + if (len > 0) { + // Position and length encoded in one smi. + int pos = len >> 11; + len &= 0x7ff; + if (pos + len > special_length) { + return Top::Throw(Heap::illegal_argument_symbol()); + } + position += len; + } else { + // Position and length encoded in two smis. + position += (-len); + // Get the position and check that it is also a smi. + i++; + if (i >= array_length) { + return Top::Throw(Heap::illegal_argument_symbol()); + } + Object* pos = fixed_array->get(i); + if (!pos->IsSmi()) { + return Top::Throw(Heap::illegal_argument_symbol()); + } } - position += len; } else if (elt->IsString()) { String* element = String::cast(elt); int element_length = element->length(); diff --git a/src/string.js b/src/string.js index d2d6e969df..bb2ad4f2a0 100644 --- a/src/string.js +++ b/src/string.js @@ -1,4 +1,4 @@ -// Copyright 2006-2008 the V8 project authors. All rights reserved. +// Copyright 2006-2009 the V8 project authors. All rights reserved. // Redistribution and use in source and binary forms, with or without // modification, are permitted provided that the following conditions are // met: @@ -810,10 +810,13 @@ ReplaceResultBuilder.prototype.addSpecialSlice = function(start, end) { var len = end - start; if (len == 0) return; var elements = this.elements; - if (start >= 0 && len >= 0 && start < 0x80000 && len < 0x800) { + if (start < 0x80000 && len < 0x800) { elements[elements.length] = (start << 11) + len; } else { - elements[elements.length] = SubString(this.special_string, start, end); + // 0 < len <= String::kMaxLength and Smi::kMaxValue >= String::kMaxLength, + // so -len is a smi. + elements[elements.length] = -len; + elements[elements.length] = start; } } diff --git a/test/mjsunit/regress/regress-490.js b/test/mjsunit/regress/regress-490.js new file mode 100644 index 0000000000..8dd8959171 --- /dev/null +++ b/test/mjsunit/regress/regress-490.js @@ -0,0 +1,48 @@ +// Copyright 2009 the V8 project authors. All rights reserved. +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following +// disclaimer in the documentation and/or other materials provided +// with the distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived +// from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// See: http://code.google.com/p/v8/issues/detail?id=490 + +var kXXX = 11 +// Build a string longer than 2^11. See StringBuilderConcatHelper and +// Runtime_StringBuilderConcat in runtime.cc and +// ReplaceResultBuilder.prototype.addSpecialSlice in string.js. +var a = ''; +while (a.length < (2 << 11)) { a+= 'x'; } + +// Test specific for bug introduced in r3153. +a.replace(/^(.*)/, '$1$1$1'); + +// More generalized test. +for (var i = 0; i < 10; i++) { + var b = ''; + for (var j = 0; j < 10; j++) { + b += '$1'; + a.replace(/^(.*)/, b); + } + a += a; +}