39438 - Malformed RegExp literals cause entire JavaScript file to not be parsed

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[aw]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.01; Windows NT)
BuildID:    2000051609

A malformed RegExp literal does not trigger any error message, and causes the 
entire JavaScript file to not be parsed.

Reproducible: Always
Steps to Reproduce:
1. Run the attached file using xpcshell: xpcshell bad-re.js
2. Edit the file and comment out the reBad literal, and uncomment the reCorrect 
literal.
3. Run the edited file in xpcshell again.

Actual Results:
Step 1. The bad RegExp caused the file to not be parsed - none of the dump() 
statements executed etc.
Step 3. The file runs fine when the RegExp is corrected.

Expected Results:  The bad RegExp should have failed to parse and generated 
some kind of error.

This also occurs when a JavaScript file with a bad RegExp is referenced from a 
XUL source file.

Comment 1

[aw]

Attachment: JavaScript test case with malformed and correct RegExp literals

Comment 2

[Asa Dotzler [:asa]]

setting status to NEW

Comment 3

[Phil Schwartau]

Sorry for the delay - taking over QA for the JavaScript Engine group -

Confirming bug. You can use the reporter's test cases in the ordinary JS Shell 
by replacing the function "dump" with the function "print".

Comment 4

[rogerl (gone)]

The problem here is that after the RE has been lexed it gets sent to the  RegExp 
parser. This detects and reports the error through the usual ReportErrorNumber() 
mechanism - i.e. sets an execption in the context. The RegExp parser also 
returns NULL which triggers an abend through the language parser, ultimately 
reaching the CompileScript level as a JS_FALSE value. At this point the API just 
returns and the error report never sees the light of day. How about if we add 
js_ReportUncaughtException() after the NewRegExp call in the scanner? Or does 
that seem too limited a fix?

Comment 5

[Brendan Eich [:brendan]]

Shouldn't any report-uncaught-exception magic happen at the highest level, in 
the API entry point code?

It does seem odd to me that JS_CompileUCScriptForPrincipals (the common 
subroutine of all the compile-script API entry points) sets a null error 
reporter around the CompileTokenStream call, then does nothing to recover a 
report from any uncaught exception.  McCabe, what's the deal?

/be

Comment 6

[Brendan Eich [:brendan]]

Anyone have a clue?  I still think some code layer near the API entry point
should recover an uncaught exception and report it.  In fact, I would say that
the code I cited previously, which intentionally sets a null error reporter,
should then worry about uncaught exceptions.  But I crave mccabe's insight.

/be

Comment 7

[Patrick C. Beard]

I think this is important, but well formed code is not affected. Marking 
[nsbeta3-].

Comment 8

[Mike McCabe]

The problem might be that the regexp parse failure is reported via
js_ReportUncaughtException instead of js_ReportCompileErrorNumber (as with
unterminated regexp, a few lines above.)

js_ReportCompileErrorNumber checks cx->interplevel to decide whether to directly
call the JS error reporter if this is a toplevel compile, or to package the
error as an exception.  In the case of a toplevel compile, we wanto to call the
embedding-supplied error reporter directly.

Regexp compilation should either do something equivalent, or communicate the
error to the scanner in such a way that the scanner can call
js_ReportCompileErrorNumber.

Comment 9

[Brendan Eich [:brendan]]

Attachment: fix attempt, needs r= love

Comment 10

[Brendan Eich [:brendan]]

I get it: all the JS_Compile* entry points report compile-time errors promptly
only if the underlying code goes through js_ReportCompileErrorNumber at top
level (cx->interpLevel == 0), else they turn those errors into exceptions. 
Right?

My patch attempts to preserve that behavior with minimal change.  McCabe's
suggestion of propagating information back from the regexp parser to jsscan.c,
so that js_GetToken can call js_ReportCompileErrorNumber, sounds better, because
it could give an error indicator with source context, pointing into the middle
of a regexp literal at the offending sub-token.  But that sounds like work!

/be

Comment 11

[Mike McCabe]

Brendan -

Might make sense to land your intermediate-fix.  I'm guessing that passing ts
through to the regexp compiler where possible (and adding it to a bunch of
parameter lists) won't be as hard as all that, but it might be too pervasive for
these times.  This'd also require teaching js_ReportCompileErrorNumber about a
null ts.

Comment 12

[rogerl (gone)]

Umm, oops. I just realized that I left in my experimental fix for this when I 
was checking in the --> as line comment change. Would you be so kind as to blow 
it off (line 1146) when you patch?

Comment 13

[Mike McCabe]

Attaching diffs that modify js_ReportCompileError to tolerate a null
tokenstream, and add a tokenstream (possibly null) to the RegExp compiler state.
regexp compile error calls are switched to js_ReportCompileError.  I've tested
in js, and the js_ReportCompileError distinction between compiletime and
execution time seems to work.

Diffs are against JS_150_RC2 branch.

Comment 14

[Mike McCabe]

Attachment: modify jsregexp.c to pass along a tokenstream for error reporting

Comment 15

[Brendan Eich [:brendan]]

Righteous, mccabe!  ra=[e-mail], get it in.

/be

Comment 16

[Mike McCabe]

Fix checked in.