197451 - Nested quantifiers in regexp should be a syntax error

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Keith Page]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3) Gecko/20030312

/1{0,1}\({0,1}\d{3}\){0,1}{0,1}\-{0,1}\d{3}\-{0,1}\d{4}/

Notice the repeatition of the {0,1} half way through the above statement,
mozilla constantly moved forward and never caught this problem, and the page
worked fully within mozilla, however when trying the same page in ie it would
constantly fail at this point. Mozilla should have noticed this error

Reproducible: Always

Steps to Reproduce:
1. repeat any section regular expression range code, mozilla never sees it, ie
always does
2.
3.
Actual Results:  
mozilla submits fine, ie fails

Expected Results:  
should have failed and returned the line as invalid

Comment 1

[Robert Ginda]

The JavaScript debugger is an application you use to debug JavaScript, see
http://www.mozilla.org/projects/venkman/.  This has nothing to do with the debugger.

Here's a simplified interaction...
js> "XXXXX".match(/X{1,2}/)
XX
js> "XXXXX".match(/X{1,2}{1,2}/)
XXXX

And here's what perl has to say about it...
$ perl -e '"XXXXX" =~ /(X{1,2})/; print "$1\n"'
XX
$ perl -e '"XXXXX" =~ /(X{1,2}{1,2})/; print "$1\n"'
Nested quantifiers in regex; marked by <-- HERE in m/(X{1,2}{ <-- HERE 1,2})/ at
-e line 1.

Comment 2

[Phil Schwartau]

Note: we already had a testcase in the JS testsuite to enforce
SyntaxErrors on nested quantifiers. Unfortunately, I had only
put cases treating the *, +, and ? operators. I have now added
cases covering the {DecimalDigits} quantifier, as well:

    mozilla/js/tests/ecma_3/RegExp/regress-188206.js

The testcase is failing not only in the current SpiderMonkey shell,
but also in Rhino, and in SpiderMonkey even with the latest patch
for bug 85721 applied (the big RegExp re-write). So that patch, as
well as Rhino (where the rewrite has already been checked in), will
have to be modified. My apologies for not having testing this case!

The Rhino shell fails on different sections of the test than the
ones I've added for this bug. That may have to do with the change
we made in allowing braces, unescaped, as literal characters in
regexp patterns. Even though this is ECMA-incorrect, we decided
to follow Perl on this. It looks like Rhino never got that change.

See bug 188206; bug 85721 comment 48; bug 190685

Comment 3

[Brendan Eich [:brendan]]

Need an error report for this case.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

ECMA seems not to allow sentences that have quantifiers chained off the same
term, so this patch replicates a js_ReportCompileErrorNumber call.  If possible
the code should consolidate the BAD_QUANTIFIER error detection.  For now (1.7
final), this will do.

/be

Comment 5

[Brendan Eich [:brendan]]

I think this is good for 1.7 final.  Reviewers will confirm, nominating bug.

/be

Comment 6

[Brendan Eich [:brendan]]

Oops, I meant result = JS_FALSE, not result = NULL, there in the first hunk. 
Please review accordingly.

/be

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 148557 [details] [diff] [review]
proposed fix

aye. sr=shaver.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 148557 [details] [diff] [review]
proposed fix

One level or review only for js/src, so I checked into the trunk.  Going for
1.7 final approval, igor feel free to r+.

/be

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 148557 [details] [diff] [review]
proposed fix

a=asa (on behalf of drivers) for checkin to 1.7

Comment 10

[Brendan Eich [:brendan]]

Fixed on 1.7 branch and trunk -- ben and mscott will update the aviary1.0 branch
later, they say.

/be

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed.