[Webkit-unassigned] [Bug 131704] Simple ES6 feature:String prototype additions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=131704


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #237169|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #26 from Darin Adler <darin at apple.com>  2014-08-26 20:37:32 PST ---
(From update of attachment 237169)
View in context: https://bugs.webkit.org/attachment.cgi?id=237169&action=review

Thanks for working so hard on this. Sorry to keep coming up with new things, but I realized a few issues i had overlooked earlier.

Also, this overloading problem is a bad thing to introduce into the String class. We really don’t want to avoid it by simply having mysterious additional "true" arguments at every call site; it’s simply an ugly pattern. I think the direction we will want to go to clean this up is that instead of adding more functions to String, we will want to add and use StringView::contains/startsWith/endsWith and use StringView::substring to restrict the ranges of the searches. When we do that we will then get rid of these new overloads we just added. I suppose it’s OK to land them like this for now, but since I do see some difficulty here in writing those functions correctly without overflow problems we might want to just write the StringView ones instead.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1571
> +    String stringToSearchIn = thisValue.toString(exec)->value(exec);
> +    String matchString = a0.toString(exec)->value(exec);

After each of these lines we need:

    if (exec->hadException())
        return JSValue::encode(jsUndefined());

That’s because toString can end up calling valueOf, which can raise an exception. It’s tricky to construct test cases that cover this, but it can be done by constructing multiple objects with valueOf functions, one that raises an exception and others that have side effects. The trick is that if one valueOf function raises an exception we can’t be seen calling the second one or the third one, since toInt32 can also end up calling valueOf. The test needs to check that the side effects of the subsequent valueOf functions do not occur when an earlier one raises an exception. We have very little test coverage for this particular fine point of getting these functions right. Making them work properly is relatively straightforward as long as we are careful to evaluate the arguments in the correct order and return if an exception occurs. Strictly speaking we also want to decide if thisValue is converted to a string before or after we check the type of argument 0.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1590
> +    String stringToSearchIn = thisValue.toString(exec)->value(exec);

Needs a hadException return statement as above.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1593
> +    String matchString = a0.toString(exec)->value(exec);

Needs a hadException return statement as above.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1615
> +    String stringToSearchIn = thisValue.toString(exec)->value(exec);

Needs a hadException return statement as above.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1616
> +    String matchString = a0.toString(exec)->value(exec);

Needs a hadException return statement as above.

> Source/WTF/wtf/text/StringImpl.cpp:1405
> +    ASSERT(matchString);

This assertion is incorrect and should be removed. References can’t be null. I’m not sure why this compiles.

> Source/WTF/wtf/text/StringImpl.cpp:1407
> +    if (matchString.length() + startOffset > length())
> +        return false;

This can overflow. It needs to be written so it is not subject to overflow. For example, matchString can be 1 character long and startOffset can be 0xFFFFFFFF. Then we’ll hit the assertion inside equalInner. Please write the code in a way that is not susceptible to overflow.

> Source/WTF/wtf/text/StringImpl.cpp:1409
> +    return equalInner(this, startOffset, reinterpret_cast<const char*>(matchString.characters8()), 
> +        matchString.length(), caseSensitive);

This won’t work if the string is not an 8-bit string.

> Source/WTF/wtf/text/StringImpl.cpp:1438
> +    ASSERT(matchString);

This assertion is incorrect and should be removed. References can’t be null. I’m not sure why this compiles.

> Source/WTF/wtf/text/StringImpl.cpp:1441
> +    int startOffset = endOffset - matchString.length();
> +    if (startOffset < 0)
> +        return false;

This can overflow. It needs to be written so it is not subject to overflow. For example, matchString can be 1 character long and endOffset can be 0x80000000. Then startOffset will be 0x7FFFFFFF and we’ll hit the assertion inside equalInner. Please write the code in a way that is not susceptible to overflow.

> Source/WTF/wtf/text/StringImpl.cpp:1443
> +    return equalInner(this, startOffset, reinterpret_cast<const char*>(matchString.characters8()), 
> +        matchString.length(), caseSensitive);

This won’t work if the string is not an 8-bit string.

> LayoutTests/js/script-tests/string-contains.js:1
> +description("This test checks the ES6 string functions startsWith(), endsWith() and contains().");

We need more edge cases here. These tests would miss all the problems with the range checking overflow in the functions.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.