346090 - crash with this javascript regexp [@ js_NewStringCopyN]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Priit Laes]

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]

Attachment: tester.html

The embedded Javascript makes Gecko to crash.

Comment 2

[Daniel Veditz [:dveditz]]

TB21493875

Comment 3

[Bob Clary [:bc] (inactive)]

confirm crash in 1.8 (TB21498432E), trunk (TB21498498G) with todays builds on winxp.

Comment 4

[Daniel Veditz [:dveditz]]

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) (inactive)]

(sorry, nuked too much there).

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

FWIW, this is another minimal quantifier bug.

Comment 7

[Daniel Veditz [:dveditz]]

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]

Attachment: 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]

Attachment: Adding a comment and fixing a couple typos resulting from rename

Comment 10

[Brian Crowder]

Attachment: A smaller js-shell usable test case

This should go into a test suite somewhere somehow.

Comment 11

[Brian Crowder]

Attachment: smaller still

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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)]

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]]

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]]

Attachment: 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]]

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]

Suitable for 1.8 branch?  If so please nom as we'd like it there...

Comment 18

[Brian Crowder]

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]

Comment on attachment 235710 [details] [diff] [review]
patch that I landed on the 1.8.0 branch

a=schrep

Comment 20

[Brian Crowder]

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]]

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]

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] (inactive)]

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] (inactive)]

verified fixed 20061010 1.8 windows/mac*/linux, 1.9 windows/linux