Skip to content

Commit

Permalink
String.prototype.includes incorrectly returns false when string is em…
Browse files Browse the repository at this point in the history
…pty and position is past end of string

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

Reviewed by Alexey Shvayka.

Added length clamping to the Int32 special case of the startsWith
and includes functions. These functions could be optimized for all sorts
of cases including not fetching string lengths multiple times, and even
optimizing rope cases to not require resolving the whole rope, but for now
didn't worry about any of that, just added that bit of extra checking.

* LayoutTests/js/script-tests/string-includes.js: Added test cases.
* LayoutTests/js/string-includes-expected.txt: Added expected result.

* Source/JavaScriptCore/runtime/StringPrototype.cpp:
(JSC::stringProtoFuncStartsWith): Added clamping to the Int32 special case.
(JSC::stringProtoFuncEndsWith): Refactored clamping to match startsWith
and includes, which removes one redudant clamp.
(JSC::stringIncludesImpl): Added clamping to the Int32 special case.

Canonical link: https://commits.webkit.org/254319@main
  • Loading branch information
darinadler committed Sep 9, 2022
1 parent aef5784 commit ebf196e
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 13 deletions.
9 changes: 9 additions & 0 deletions LayoutTests/js/script-tests/string-includes.js
Expand Up @@ -38,6 +38,9 @@ shouldBeTrue("'abc'.includes('ab', -Infinity)");
shouldBeFalse("'abc'.includes('cd', -Infinity)");
shouldBeTrue("'abc'.includes('ab', 0)");
shouldBeFalse("'abc'.includes('cd', 0)");
shouldBeTrue("'abc'.includes('', 3)")
shouldBeTrue("'abc'.includes('', 4)")
shouldBeTrue("'abc'.includes('', Infinity)")

// Test startsWith
shouldBe("String.prototype.startsWith.name", "'startsWith'");
Expand Down Expand Up @@ -75,6 +78,9 @@ shouldBeTrue("'abc'.startsWith('b', 1)");
shouldBeFalse("'abc'.startsWith('b', 2)");
shouldBeTrue("'abc'.startsWith('c', 2)");
shouldBeFalse("'abc'.startsWith('a', Math.pow(2, 33))");
shouldBeTrue("'abc'.startsWith('', 3)")
shouldBeTrue("'abc'.startsWith('', 4)")
shouldBeTrue("'abc'.startsWith('', Infinity)")

// Test endsWith
shouldBe("String.prototype.endsWith.name", "'endsWith'");
Expand Down Expand Up @@ -117,6 +123,9 @@ shouldBeFalse("'abc'.endsWith('b', 1)");
shouldBeTrue("'abc'.endsWith('b', 2)");
shouldBeFalse("'abc'.endsWith('bc', 2)");
shouldBeTrue("'abc'.endsWith('bc', 3)");
shouldBeTrue("'abc'.endsWith('', 3)")
shouldBeTrue("'abc'.endsWith('', 4)")
shouldBeTrue("'abc'.endsWith('', Infinity)")

// Call functions with an environment record as 'this'.
shouldThrow("(function() { var f = String.prototype.startsWith; (function() { f('a'); })(); })()");
Expand Down
9 changes: 9 additions & 0 deletions LayoutTests/js/string-includes-expected.txt
Expand Up @@ -40,6 +40,9 @@ PASS 'abc'.includes('ab', -Infinity) is true
PASS 'abc'.includes('cd', -Infinity) is false
PASS 'abc'.includes('ab', 0) is true
PASS 'abc'.includes('cd', 0) is false
PASS 'abc'.includes('', 3) is true
PASS 'abc'.includes('', 4) is true
PASS 'abc'.includes('', Infinity) is true
PASS String.prototype.startsWith.name is 'startsWith'
PASS String.prototype.startsWith.length is 1
PASS 'foo bar'.startsWith('foo') is true
Expand Down Expand Up @@ -75,6 +78,9 @@ PASS 'abc'.startsWith('b', 1) is true
PASS 'abc'.startsWith('b', 2) is false
PASS 'abc'.startsWith('c', 2) is true
PASS 'abc'.startsWith('a', Math.pow(2, 33)) is false
PASS 'abc'.startsWith('', 3) is true
PASS 'abc'.startsWith('', 4) is true
PASS 'abc'.startsWith('', Infinity) is true
PASS String.prototype.endsWith.name is 'endsWith'
PASS String.prototype.endsWith.length is 1
PASS 'foo bar'.endsWith('bar') is true
Expand Down Expand Up @@ -115,6 +121,9 @@ PASS 'abc'.endsWith('b', 1) is false
PASS 'abc'.endsWith('b', 2) is true
PASS 'abc'.endsWith('bc', 2) is false
PASS 'abc'.endsWith('bc', 3) is true
PASS 'abc'.endsWith('', 3) is true
PASS 'abc'.endsWith('', 4) is true
PASS 'abc'.endsWith('', Infinity) is true
PASS (function() { var f = String.prototype.startsWith; (function() { f('a'); })(); })() threw exception TypeError: Type error.
PASS (function() { var f = String.prototype.endsWith; (function() { f('a'); })(); })() threw exception TypeError: Type error.
PASS (function() { var f = String.prototype.includes; (function() { f('a'); })(); })() threw exception TypeError: Type error.
Expand Down
27 changes: 14 additions & 13 deletions Source/JavaScriptCore/runtime/StringPrototype.cpp
Expand Up @@ -1727,11 +1727,11 @@ JSC_DEFINE_HOST_FUNCTION(stringProtoFuncStartsWith, (JSGlobalObject* globalObjec
RETURN_IF_EXCEPTION(scope, encodedJSValue());

JSValue positionArg = callFrame->argument(1);
unsigned start = 0;
unsigned length = stringToSearchIn.length();
unsigned start;
if (positionArg.isInt32())
start = std::max(0, positionArg.asInt32());
start = std::min(clampTo<unsigned>(positionArg.asInt32()), length);
else {
unsigned length = stringToSearchIn.length();
start = clampAndTruncateToUnsigned(positionArg.toIntegerOrInfinity(globalObject), 0, length);
RETURN_IF_EXCEPTION(scope, encodedJSValue());
}
Expand Down Expand Up @@ -1760,28 +1760,29 @@ JSC_DEFINE_HOST_FUNCTION(stringProtoFuncEndsWith, (JSGlobalObject* globalObject,
String searchString = a0.toWTFString(globalObject);
RETURN_IF_EXCEPTION(scope, encodedJSValue());

unsigned length = stringToSearchIn.length();

JSValue endPositionArg = callFrame->argument(1);
unsigned end = length;
if (endPositionArg.isInt32())
end = std::max(0, endPositionArg.asInt32());
else if (!endPositionArg.isUndefined()) {
unsigned length = stringToSearchIn.length();
unsigned end;
if (endPositionArg.isUndefined())
end = length;
else if (endPositionArg.isInt32())
end = std::min(clampTo<unsigned>(endPositionArg.asInt32()), length);
else {
end = clampAndTruncateToUnsigned(endPositionArg.toIntegerOrInfinity(globalObject), 0, length);
RETURN_IF_EXCEPTION(scope, encodedJSValue());
}

return JSValue::encode(jsBoolean(stringToSearchIn.hasInfixEndingAt(searchString, std::min(end, length))));
return JSValue::encode(jsBoolean(stringToSearchIn.hasInfixEndingAt(searchString, end)));
}

static EncodedJSValue stringIncludesImpl(JSGlobalObject* globalObject, VM& vm, String stringToSearchIn, String searchString, JSValue positionArg)
{
auto scope = DECLARE_THROW_SCOPE(vm);
unsigned start = 0;
auto length = stringToSearchIn.length();
unsigned start;
if (positionArg.isInt32())
start = std::max(0, positionArg.asInt32());
start = std::min(clampTo<unsigned>(positionArg.asInt32()), length);
else {
unsigned length = stringToSearchIn.length();
start = clampAndTruncateToUnsigned(positionArg.toIntegerOrInfinity(globalObject), 0, length);
RETURN_IF_EXCEPTION(scope, encodedJSValue());
}
Expand Down

0 comments on commit ebf196e

Please sign in to comment.