346237 - This regexp makes Firefox attempt to allocate 2097156096 bytes, then crash [@ JS_sprintf_append] [@ js_ExecuteRegExp][@ PushBackTrackState]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

This regexp makes Firefox attempt to allocate 2097156096 bytes, then crash [@ JS_sprintf_append] [@ js_ExecuteRegExp].

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

Before the crash, I see this:

firefox-bin(9014,0xa000ed98) malloc: *** vm_allocate(size=2097156096) failed (error code=3)
firefox-bin(9014,0xa000ed98) malloc: *** error: can't allocate region
firefox-bin(9014,0xa000ed98) malloc: *** set a breakpoint in szone_error to debug

Comment 3

[timeless]

Attachment: this seems to fix the problem for me

Comment 4

[timeless]

for reference, i have:
wXPsp2 minefield-mcsmurf xpcshell
wXPsp2 minefield-talkback minefield
linux spidermonkey js
solaris mozilla1.7 xpcshell

all of these crash as follows:
js3250!PushBackTrackState [h:\mozilla\tree-main\mozilla\js\src\jsregexp.c @ 2075]
js3250!ExecuteREBytecode
js3250!MatchRegExp
js3250!js_ExecuteRegExp
js3250!regexp_exec_sub
js3250!regexp_exec
js3250!js_Invoke
js3250!js_Interpret
js3250!js_Execute
js3250!JS_ExecuteScript
xpcshell!ProcessFile

The windows talkback is: Incident ID: 21518101
Source File, Line No.	c:\builds\tinderbox\fx-trunk-cairo\winnt_5.2_depend\mozilla\js\src\jsregexp.c, line 2075
Stack Trace 	
PushBackTrackState   ExecuteREBytecode   MatchRegExp   regexp_exec_sub   regexp_exec   js_Invoke   

I've asked jesse to review because his crash signature doesn't match mine, even though I have 3 platforms (unfortunately all x86) that crash the same way.

Comment 5

[Jesse Ruderman]

With this patch, Firefox no longer crashes, but it does hang for about a minute while it temporarily consumes 1GB of memory and attempts to allocate 2097156096 bytes at one point.

Comment 6

[Jesse Ruderman]

(Comment 0 is mostly with a nightly; comment 5 is with a debug build.)

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Wallpaper over the crash

I seem to recall writing a patch just like this one before, except I don't remember which bug it's on. The basic problem here is that minimal quantifiers are almost totally broken in combination with anything else. This patch fixes the crash by actually propagating the error from JS_ARENA_GROW_CAST. This is not a full fix for this bug.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 231100 [details] [diff] [review]
Wallpaper over the crash

Any more like that?  Grep twice.

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better wallpaper

This caught another similar problem in ExecuteREBytecode and added a couple of missing calls to JS_ReportOutOfMemory to InitMatch.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 231128 [details] [diff] [review]
Better wallpaper


>+                    if (!charSet->converted) {
>+                        if (!ProcessCharSet(gData, charSet)) {
>+                            gData->ok = JS_FALSE;
>                             return NULL;
>+                        }
>+                    }

Collapse if-if?

Also, shouldn't the innermost layer parameterized with gData that fails be required to set gData->ok = JS_FALSE?

>+    if (!gData->stateStack) {
>+        JS_ReportOutOfMemory(cx);
>         return NULL;
>+    }

No gData->ok = JS_FALSE here, guess it's not needed by callers, but still -- consistent and simple rules beat inconsistent or asymmetric/complicated rules.

OTOH small patches are better.  But perhaps the patch to enforce gData ok=false-setting rules wouldn't be much bigger.

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Even better wallpaper

This has better bookkeeping, and I think I fixed a leak in js_ExecuteRegExp, since we'd allocate our pool, but then return without finishing it.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 231178 [details] [diff] [review]
Even better wallpaper

This could go on for a while ;-).

/be

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

I checked the even better wallpaper in. I'm going to leave this bug open to fix the underlying problem. 

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 231178 [details] [diff] [review]
Even better wallpaper

This should go into 1.8.1 too.  Not sure why it was not nominated yet.  I found it by diffing trunk vs. 1.8 branch js/src.

/be

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

The better wallpaper is now checked into the 1.8 branch.

Comment 16

[Jesse Ruderman]

Should the hang get a new bug, so the fixed1.8.1 flag here (etc.) don't confuse everyone?

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> Should the hang get a new bug, so the fixed1.8.1 flag here (etc.) don't confuse
> everyone?

Yes, how about a new bug.  Is it not a dup of existing exponential regexp runtime bug or bugs?

/be

Comment 18

[Jesse Ruderman]

Filed bug 367888 for the large allocation and hang.  Marking this bug as fixed.

Comment 19

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-346237.js,v  <--  regress-346237.js
initial revision: 1.1

Comment 20

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows. 1.8.1 fails with out of memory but nothing else.