crash with this javascript regexp [@ js_NewStringCopyN]

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
11 years ago
6 years ago

People

(Reporter: Priit Laes, Assigned: Brian Crowder)

Tracking

({crash, verified1.8.0.7, verified1.8.1})

Trunk
mozilla1.8.1
x86
All
crash, verified1.8.0.7, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
Created attachment 230886 [details]
tester.html

The embedded Javascript makes Gecko to crash.
Attachment #230886 - Attachment mime type: text/plain → text/html
TB21493875
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: Security → JavaScript Engine
Ever confirmed: true
Product: Firefox → Core
QA Contact: firefox → general
Version: unspecified → 1.8 Branch

Comment 3

11 years ago
confirm crash in 1.8 (TB21498432E), trunk (TB21498498G) with todays builds on winxp.
OS: Linux → All
Version: 1.8 Branch → Trunk

Updated

11 years ago
Assignee: general → mrbkap
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1beta2

Updated

11 years ago
Status: NEW → ASSIGNED
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.
OS: All → Linux
Priority: P1 → --
Whiteboard: [sg:critical]
Target Milestone: mozilla1.8.1beta2 → ---
Version: Trunk → 1.8 Branch

Updated

11 years ago
OS: Linux → All
Priority: -- → P1
Whiteboard: [sg:critical]
Target Milestone: --- → mozilla1.8.1beta2
Version: 1.8 Branch → Trunk
(sorry, nuked too much there).
Whiteboard: [sg:critical]
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Flags: blocking1.8.1? → blocking1.8.1+
Flags: blocking1.8.0.6? → blocking1.8.0.6+

Updated

11 years ago
Summary: firefox and Epiphany both crash when using javascript → crash with this javascript regexp

Updated

11 years ago
Summary: crash with this javascript regexp → crash with this javascript regexp [@ js_NewStringCopyN]
FWIW, this is another minimal quantifier bug.

Updated

11 years ago
Target Milestone: mozilla1.8.1beta2 → mozilla1.8.1
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
Assignee: mrbkap → brendan
Status: ASSIGNED → NEW

Updated

11 years ago
Assignee: brendan → crowder
(Assignee)

Comment 8

11 years ago
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.
Attachment #235336 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #235336 - Flags: review? → review?(shaver)
(Assignee)

Comment 9

11 years ago
Created attachment 235339 [details] [diff] [review]
Adding a comment and fixing a couple typos resulting from rename
Attachment #235336 - Attachment is obsolete: true
Attachment #235339 - Flags: review?
Attachment #235336 - Flags: review?(shaver)

Updated

11 years ago
Attachment #235339 - Flags: review? → review?(shaver)
(Assignee)

Comment 10

11 years ago
Created attachment 235358 [details]
A smaller js-shell usable test case

This should go into a test suite somewhere somehow.

Updated

11 years ago
Flags: in-testsuite?
(Assignee)

Updated

11 years ago
Attachment #235358 - Attachment is obsolete: true
(Assignee)

Comment 11

11 years ago
Created attachment 235412 [details]
smaller still
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 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.
Attachment #235339 - Flags: review?(shaver) → review+
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.
Attachment #235339 - Flags: approval1.8.0.7+
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
Keywords: fixed1.8.0.7
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
Keywords: fixed1.8.0.7 → verified1.8.0.7
Keywords: crash

Comment 17

11 years ago
Suitable for 1.8 branch?  If so please nom as we'd like it there...
(Assignee)

Comment 18

11 years ago
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.
Attachment #235710 - Flags: approval1.8.1?

Comment 19

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

a=schrep
Attachment #235710 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 20

11 years ago
The only complication is that I still do not have privileges to land this.  Can you do it again, 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
Keywords: fixed1.8.1
Group: security
(Assignee)

Comment 22

11 years ago
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?
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 23

11 years ago
Checking in regress-346090.js;
/cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-346090.js,v  <--  regress-346090.js
initial revision: 1.1
done

Flags: in-testsuite? → in-testsuite+

Comment 24

11 years ago
verified fixed 20061010 1.8 windows/mac*/linux, 1.9 windows/linux
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1

Updated

10 years ago
Blocks: 379056
Crash Signature: [@ js_NewStringCopyN]
You need to log in before you can comment on or make changes to this bug.