Last Comment Bug 346090 - crash with this javascript regexp [@ js_NewStringCopyN]
: crash with this javascript regexp [@ js_NewStringCopyN]
Status: VERIFIED FIXED
[sg:critical]
: crash, verified1.8.0.7, verified1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 All
: P1 critical (vote)
: mozilla1.8.1
Assigned To: Brian Crowder
:
Mentors:
Depends on:
Blocks: 379056
  Show dependency treegraph
 
Reported: 2006-07-27 06:19 PDT by Priit Laes
Modified: 2011-06-13 10:01 PDT (History)
7 users (show)
dbaron: blocking1.8.1+
dveditz: blocking1.8.0.7+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
tester.html (1.14 KB, text/html)
2006-07-27 06:20 PDT, Priit Laes
no flags Details
two line fix, return an empty string match instead of backtracking (1.11 KB, patch)
2006-08-24 17:30 PDT, Brian Crowder
no flags Details | Diff | Review
Adding a comment and fixing a couple typos resulting from rename (1.25 KB, patch)
2006-08-24 17:44 PDT, Brian Crowder
shaver: review+
dveditz: approval1.8.0.7+
Details | Diff | Review
A smaller js-shell usable test case (117 bytes, text/plain)
2006-08-24 21:38 PDT, Brian Crowder
no flags Details
smaller still (59 bytes, text/plain)
2006-08-25 09:31 PDT, Brian Crowder
no flags Details
patch that I landed on the 1.8.0 branch (1.39 KB, patch)
2006-08-27 23:27 PDT, Brendan Eich [:brendan]
mtschrep: approval1.8.1+
Details | Diff | Review

Description Priit Laes 2006-07-27 06:19:46 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.4) Gecko/20060608 Ubuntu/dapper-security Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.4) Gecko/20060608 Ubuntu/dapper-security Firefox/1.5.0.4

Will post example file...

Reproducible: Always

Steps to Reproduce:
1.open the submitted file
2.press the popup buttons
3.observe the crash




Also happened with Epiphany on Ubuntu Dapper
Comment 1 Priit Laes 2006-07-27 06:20:49 PDT
Created attachment 230886 [details]
tester.html

The embedded Javascript makes Gecko to crash.
Comment 2 Daniel Veditz [:dveditz] 2006-07-27 11:39:57 PDT
TB21493875
Comment 3 Bob Clary [:bc:] 2006-07-27 12:07:53 PDT
confirm crash in 1.8 (TB21498432E), trunk (TB21498498G) with todays builds on winxp.
Comment 4 Daniel Veditz [:dveditz] 2006-07-27 12:23:27 PDT
In a debug build I hit the "JS_ASSERT(nbytes != 0);" in JS_Malloc, called from js_NewStringCopyN as in the talkback crash.

In a non-debug build nbytes gets reset to 1 and a small bit allocated, success! Back in js_NewCopyStringN() the reason it was trying to allocate 0 bytes is because the string length passed in from the RegExp is -1 -- oops. With malloc successful it looks like js_strncpy tries to copy 0xffffffff characters into a 1 character buffer.
Comment 5 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-07-27 12:25:15 PDT
(sorry, nuked too much there).
Comment 6 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-08-01 17:14:54 PDT
FWIW, this is another minimal quantifier bug.
Comment 7 Daniel Veditz [:dveditz] 2006-08-24 11:53:53 PDT
Brendan: any chance you could help out with a few well-placed RegExp error checks here? I assume we can't afford to check for -1 inside js_NewStringCopyN itself, but RegExp maybe could in the places that matter
Comment 8 Brian Crowder 2006-08-24 17:30:59 PDT
Created attachment 235336 [details] [diff] [review]
two line fix, return an empty string match instead of backtracking

I think this is not really The Right Thing to Do here; probably the way these matches are compiled should be modified to detect potentially empty matches and generate a specific bytecode that does nothing but capture an empty string.
Comment 9 Brian Crowder 2006-08-24 17:44:58 PDT
Created attachment 235339 [details] [diff] [review]
Adding a comment and fixing a couple typos resulting from rename
Comment 10 Brian Crowder 2006-08-24 21:38:15 PDT
Created attachment 235358 [details]
A smaller js-shell usable test case

This should go into a test suite somewhere somehow.
Comment 11 Brian Crowder 2006-08-25 09:31:33 PDT
Created attachment 235412 [details]
smaller still
Comment 12 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-08-25 09:35:20 PDT
Brian and I want to try to avoid the condition that causes this negative length, rather than just protect against it occurring, but the existing patch is minimal-risk and probably a good candidate for 1.8.0.7 because of that.
Comment 13 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-08-25 09:47:59 PDT
Comment on attachment 235339 [details] [diff] [review]
Adding a comment and fixing a couple typos resulting from rename

r=shaver as per the previous comment in case you want it ASAP for 1.8.0.7, though we'd like something more fundamental for 1.8.1 and trunk, IMO.
Comment 14 Daniel Veditz [:dveditz] 2006-08-25 11:51:22 PDT
Comment on attachment 235339 [details] [diff] [review]
Adding a comment and fixing a couple typos resulting from rename

approved for 1.8.0 branch, a=dveditz for drivers

Please land this weekend.

If you get a more robust fix for 1.8.1 we'll look at doing that in 1.8.0.8 instead of this.
Comment 15 Brendan Eich [:brendan] 2006-08-27 23:27:57 PDT
Created attachment 235710 [details] [diff] [review]
patch that I landed on the 1.8.0 branch

Mainly style policing (80th column, major comment, no extra parens for + vs. ==), but also a FIXME in the comment citing the bug.

/be
Comment 16 alice nodelman [:alice] [:anode] 2006-08-28 14:59:29 PDT
https://bugzilla.mozilla.org/attachment.cgi?id=230886&action=view should not crash.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060828 Firefox/1.5.0.7pre

verified 1.8.0.7
Comment 17 Mike Schroepfer 2006-09-11 16:28:49 PDT
Suitable for 1.8 branch?  If so please nom as we'd like it there...
Comment 18 Brian Crowder 2006-09-11 16:44:58 PDT
Comment on attachment 235710 [details] [diff] [review]
patch that I landed on the 1.8.0 branch

Should be pretty safe and seems to resolve the problem.  I think the only reason this bug remains open (or should) is to remind me that there is a Better Fix to be done.
Comment 19 Mike Schroepfer 2006-09-11 18:51:56 PDT
Comment on attachment 235710 [details] [diff] [review]
patch that I landed on the 1.8.0 branch

a=schrep
Comment 20 Brian Crowder 2006-09-11 20:10:29 PDT
The only complication is that I still do not have privileges to land this.  Can you do it again, Brendan?
Comment 21 Brendan Eich [:brendan] 2006-09-11 21:37:48 PDT
Checking in jsregexp.c;
/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.99.2.18; previous revision: 3.99.2.17
done
Comment 22 Brian Crowder 2006-09-27 23:38:25 PDT
How does this bug become fixed/verified?  If there's a Schoolhouse Rocks song about it, that'd be helpful.

Also can the attachment from comment #11 be made into a test-case?
Comment 23 Bob Clary [:bc:] 2006-10-10 02:01:01 PDT
Checking in regress-346090.js;
/cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-346090.js,v  <--  regress-346090.js
initial revision: 1.1
done

Comment 24 Bob Clary [:bc:] 2006-10-11 02:25:00 PDT
verified fixed 20061010 1.8 windows/mac*/linux, 1.9 windows/linux

Note You need to log in before you can comment on or make changes to this bug.