Bug 209573 – Quantifiers with min/max values exceeding 2 ** 32 - 1 should be valid

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Alexey Shvayka]

Test case:
  /b{99999999999999999999,}/.test("a")

Expected:
  false

Actual:
  SyntaxError thrown

ECMA262: https://tc39.es/ecma262/#sec-quantifier (no limit specified)
Test262: https://test262.report/browse/built-ins/RegExp/quantifier-integer-limit.js

[Alexey Shvayka]

Created attachment 394623 [details]
Patch

[Darin Adler]

Comment on attachment 394623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394623&action=review

> Source/JavaScriptCore/yarr/YarrPattern.cpp:956
> -        ASSERT(minimumInputSize != UINT_MAX);
> +        ASSERT(minimumInputSize <= UINT_MAX);

This assertion is meaningless. Of course an unsigned is <= unsigned maximum. So this change is removing the assertion entirely. Which is fine, but just remove it.

[Alexey Shvayka]

Created attachment 394628 [details]
Patch

Set reviewer and remove useless ASSERT.

[Alexey Shvayka]

Created attachment 394630 [details]
Patch

Set reviewer and remove useless ASSERT.

[Darin Adler]

Comment on attachment 394623 [details]
Patch

One other thought about testing. It might be helpful to test with values like 2^32 and 2^64 to make they work as infinity and don’t get truncated and turn into 0 or something like that.

[Alexey Shvayka]

Created attachment 394643 [details]
Patch

Test 2^32 and 2^64 values in quantifiers.

[Darin Adler]

Comment on attachment 394643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394643&action=review

> LayoutTests/fast/regex/script-tests/overflow.js:14
> +var regexp4 = /[^a$]{4294967296}/; // 2^32
> +shouldBe("regexp4.exec('')", 'null');

This doesn’t clarify whether this is acting like {<infinity>} or {1}. Maybe we need a slightly different test for that?

[Michael Saboff]

Comment on attachment 394643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394643&action=review

Could you add a tests for something like new var regex7 = /a{2147483648}b{2147483648}c{2147483648}"/ and other tests where the minimum number of characters needed to match is greater than 2^32?  We should get an early "pattern exceeds string length limits" syntax error for such tests.  If not, then we need to add appropriate checks in the parser, JIT and interpreter to make sure we don't underflow.

> Source/JavaScriptCore/ChangeLog:10
> +        possible overflows.

Please document that this change doesn't remove the 2^32-1 quantity limit required by the underlying matching engines.  That limit is enforced in the YARR parser's consumeNumber() function, which returns quantifyInfinite (UINT_MAX) on values that overflow an unsigned value.

This change also introduces minor bugs where a counted atoms with values greater than 2^32-1 are silently truncated.  For example a{2^32+1,2^32} (the min count is greater than the max count) is silently truncates this to a{2^32-1).  And a{2^32,2^32+2}, a variable range becomes a fixed range of a{2^32-1}.

> Source/JavaScriptCore/ChangeLog:13
> +        limits on min/max values. This patch aligns JSC with V8 and SpiderMonkey.

Even though this patch aligns JSC's counted quantity limit syntax checks with other JS engines, do those other engines support matching counted atoms with counts >= 2^32?

> Source/JavaScriptCore/yarr/YarrPattern.cpp:-956
> -        ASSERT(minimumInputSize != UINT_MAX);

This assert is checking that minimumInputSize was set to a lower value in the for loop above as it starts out as UINT_MAX ~920.

> LayoutTests/fast/regex/script-tests/overflow.js:19
> +var regexp6 = new RegExp('a{0,' + Math.pow(2, 64) + '}');

This test is somewhat meaningless as the parser with truncate the max quantity to 2^31-1.

[Michael Saboff]

Comment on attachment 394643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394643&action=review

>> LayoutTests/fast/regex/script-tests/overflow.js:19
>> +var regexp6 = new RegExp('a{0,' + Math.pow(2, 64) + '}');
> 
> This test is somewhat meaningless as the parser with truncate the max quantity to 2^31-1.

This comment should read "This test is somewhat meaningless as the parser will truncate the max quantity to 2^32-1."

[Darin Adler]

Comment on attachment 394643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394643&action=review

>> Source/JavaScriptCore/yarr/YarrPattern.cpp:-956
>> -        ASSERT(minimumInputSize != UINT_MAX);
> 
> This assert is checking that minimumInputSize was set to a lower value in the for loop above as it starts out as UINT_MAX ~920.

Right, but now we also use UINT_MAX to mean infinity.

If we really wanted the assertion we’d have to make minimumInputSize an Optional or something like that. I think it’s OK to lose this assertion.

[Alexey Shvayka]

*** This bug has been marked as a duplicate of bug 276306 ***