RegExp.prototype.test bug with '@'

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[kangax]

The following regexp returns `true` in latest nightly (r37056), but should return `false` (and does return `false` in Safari 3.1)

(/^[,:{}\[\]0-9.\-+Eaeflnr-u \n\r\t]*$/).test('@');

Thank you.

[Alexey Proskuryakov]

Confirmed as a regression.

[Gavin Barraclough]

There are two problems here, relating to the handling of hyphens in character ranges.

(1) In order to prevent [a\-c] from being treated as [a-c], we're forcing a flush after the hyphen has been added to the class.  Unfortunately this will give incorrect behaviour on the regexp [\--@], which should generate the range '-'..'@', but instead due to the flush will add '-' twice and '@' once to the class, all treated as separate characters.  The easy solution looks like it would be to flush prior to adding '\-' to the class, but this is also incorrect, since [+-\-] would now add separate characters rather than a range.  In order to prevent '\-' as being treated as a regular '-', but to still allow it itself to be a part of a range, we should flush the class under construction before adding the hyphen only if m_isPendingDash is not already set (since if m_isPendingDash is set this is a range, and the escaped hyphen is the last character in the range).

(2) The second bug is that we're not reseting m_isPendingDash when we flush.  So, given the regular expression [oh-\shoot, we'll add the 'o', be in a state that the 'h' and the hyphen are pending, when we see the character class next we'll flush, adding the 'h' and the hyphen, putting ourselves in an illegal state where m_isPendingDash is set but m_charBuffer isn't.  When the subsequent 'h' and 'o' are added to the set these will be treated as a range, since m_isPendingDash was not cleared.  This bug is not restricted to escaped hypens, and can be defended against will an assert when adding new chars that we're not in a state with m_isPendingDash set but not m_charBuffer.

In combination, the bug in this specific regexp is that the range '+'..'E' is getting added to the class.

patch coming.

[Gavin Barraclough]

Created attachment 23942 [details]
The patch

[Darin Adler]

Comment on attachment 23942 [details]
The patch

The fix looks fine, but we need to add test cases to LayoutTests for this. The patch should include the new tests and test results as well as the bug fix.

review- for now because of the lack of a test.

[Gavin Barraclough]

Created attachment 24013 [details]
The patch - now with added layouttesty goodness!

[Gavin Barraclough]

Created attachment 24015 [details]
Layout tests have Changelogs too, y'know.  D'oh.

[Darin Adler]

Comment on attachment 24015 [details]
Layout tests have Changelogs too, y'know.  D'oh.

+//var regexp03a = /[1-3]+/.exec("[-]");
+//shouldBe('regexp03a.toString()', '"[-]"');
+//var regexp03a = /[\--o]+/.exec("-ok");
+//shouldBe('regexp03a.toString()', '"[-]"');

Why are those commented out?

Normally, we like to include tests even if they show failures.

r=me

[Gavin Barraclough]

The commented out tests were incorrect, I've now deleted them (correct versions of these checks were these:

var regexp03a = /[\--0]+/.exec(",-.01");
shouldBe('regexp03a.toString()', '"-.0"');
var regexp03b = /[+-\-]+/.exec("*+,-.");
shouldBe('regexp03b.toString()', '"+,-"');

- and these are now commented.)

[Gavin Barraclough]

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/wrec/CharacterClassConstructor.cpp
Sending        JavaScriptCore/wrec/CharacterClassConstructor.h
Sending        JavaScriptCore/wrec/WREC.cpp
Sending        JavaScriptCore/wrec/WREC.h
Sending        LayoutTests/ChangeLog
Adding         LayoutTests/fast/js/regexp-ranges-and-escaped-hyphens-expected.txt
Adding         LayoutTests/fast/js/regexp-ranges-and-escaped-hyphens.html
Adding         LayoutTests/fast/js/resources/regexp-ranges-and-escaped-hyphens.js
Transmitting file data .........
Committed revision 37194.