Bug 209573

Created attachment 394623 [details]
Patch

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.

Created attachment 394630 [details]
Patch

Set reviewer and remove useless ASSERT.

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.

Created attachment 394643 [details]
Patch

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

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?

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.

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."

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.

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