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
:
: Jason Orendorff [:jorendorff]
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 | Splinter 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 | Splinter 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 | Splinter Review

Description User image 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 User image Priit Laes 2006-07-27 06:20:49 PDT
Created attachment 230886 [details]
tester.html

The embedded Javascript makes Gecko to crash.
Comment 2 User image Daniel Veditz [:dveditz] 2006-07-27 11:39:57 PDT
TB21493875
Comment 3 User image 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 User image 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 User image Blake Kaplan (:mrbkap) 2006-07-27 12:25:15 PDT
(sorry, nuked too much there).
Comment 6 User image Blake Kaplan (:mrbkap) 2006-08-01 17:14:54 PDT
FWIW, this is another minimal quantifier bug.
Comment 7 User image 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 User image 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 User image 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 User image 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 User image Brian Crowder 2006-08-25 09:31:33 PDT
Created attachment 235412 [details]
smaller still
Comment 12 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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.