Closed Bug 346237 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.8.1beta2

People

(Reporter: jruderman, Assigned: mrbkap)

Details

(Keywords: crash, fixed1.8.1, testcase)

Crash Data

Attachments

(2 files, 3 obsolete files)

This regexp makes Firefox attempt to allocate 2097156096 bytes, then crash [@ JS_sprintf_append] [@ js_ExecuteRegExp].
Attached file testcase
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
Attachment #231076 - Flags: review?(mrbkap)
Attachment #231076 - Flags: review?(jruderman)
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.
Summary: This regexp makes Firefox attempt to allocate 2097156096 bytes, then crash [@ JS_sprintf_append] [@ js_ExecuteRegExp] → This regexp makes Firefox attempt to allocate 2097156096 bytes, then crash [@ JS_sprintf_append] [@ js_ExecuteRegExp][@ PushBackTrackState]
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.
Attachment #231076 - Flags: review?(jruderman)
(Comment 0 is mostly with a nightly; comment 5 is with a debug build.)
Attached patch Wallpaper over the crash (obsolete) — Splinter Review
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.
Assignee: general → mrbkap
Attachment #231076 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #231100 - Flags: review?(brendan)
Attachment #231076 - Flags: review?(mrbkap)
OS: Mac OS X 10.4 → All
Priority: -- → P1
Hardware: Macintosh → All
Target Milestone: --- → mozilla1.8.1beta2
Comment on attachment 231100 [details] [diff] [review]
Wallpaper over the crash

Any more like that?  Grep twice.

/be
Attachment #231100 - Flags: review?(brendan) → review+
Attached patch Better wallpaper (obsolete) — Splinter Review
This caught another similar problem in ExecuteREBytecode and added a couple of missing calls to JS_ReportOutOfMemory to InitMatch.
Attachment #231100 - Attachment is obsolete: true
Attachment #231128 - Flags: review?(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
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.
Attachment #231128 - Attachment is obsolete: true
Attachment #231178 - Flags: review?(brendan)
Attachment #231128 - Flags: review?(brendan)
Comment on attachment 231178 [details] [diff] [review]
Even better wallpaper

This could go on for a while ;-).

/be
Attachment #231178 - Flags: review?(brendan) → review+
I checked the even better wallpaper in. I'm going to leave this bug open to fix the underlying problem. 
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
Attachment #231178 - Flags: approval1.8.1?
Flags: blocking1.8.1?
Flags: blocking1.8.1? → blocking1.8.1+
Attachment #231178 - Flags: approval1.8.1? → approval1.8.1+
The better wallpaper is now checked into the 1.8 branch.
Keywords: fixed1.8.1
Should the hang get a new bug, so the fixed1.8.1 flag here (etc.) don't confuse everyone?
(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
Filed bug 367888 for the large allocation and hang.  Marking this bug as fixed.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-346237.js,v  <--  regress-346237.js
initial revision: 1.1
Flags: in-testsuite+
verified fixed 1.9.0 linux/mac*/windows. 1.8.1 fails with out of memory but nothing else.
Status: RESOLVED → VERIFIED
Crash Signature: [@ JS_sprintf_append] [@ js_ExecuteRegExp] [@ PushBackTrackState]
You need to log in before you can comment on or make changes to this bug.