826581 - Crash [@ js::RegExpShared::compile]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

try {
    x = function()   {}
    for (y = 0; y < 64; y++) {
        x += x
    }
} catch (e) {}
x.replace(x, x, "gy")

crashes js opt shell on m-c changeset 33064e13c3fd without any CLI arguments at js::RegExpShared::compile when the testcase is passed in as a CLI argument.

s-s and assuming sec-critical because weird memory address 0x913fb008 is being accessed.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   116117:d229008d60ac
user:        Sean Stangl
date:        Wed Dec 12 17:23:04 2012 -0800
summary:     Bug 808245, Part 4/6 - Compile RegExpShared at execution time. r=dvander

Comment 1

[Sean Stangl [:sstangl]]

This is a great fuzz result. The problem is that the code currently assumes that the RegExp JSAtom source, stored in the RegExpShared, is kept rooted by at least one RegExpObject. This assumption is incorrect in the context of our "flags" extension to str_replace(), which converts a string to a regular expression without rooting it.

The solution is to have the RegExpGuard hold a root to the source. This gets a bit tricky, because Bug 820124 wants there to be a RegExpGuard in the RegExpStatics -- so there will have to be a second "heap-safe" RegExpGuard-like class just for that case.

Comment 2

[Alex Keybl [:akeybl]]

Are you the right owner for this sg:crit regression? If not, please re-assign appropriately.

Comment 3

[Sean Stangl [:sstangl]]

Yes, I'm the right owner. I have a fix already, but I'm going to manually verify all the RegExp rooting sites to make sure this bug doesn't occur again.

Comment 4

[Sean Stangl [:sstangl]]

Attachment: Root RegExp source for the lifetime of RegExpShared

Comment 5

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d9055884036

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7d9055884036

Needs landing on aurora.

Comment 7

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 8

[Sean Stangl [:sstangl]]

Comment on attachment 698851 [details] [diff] [review]
Root RegExp source for the lifetime of RegExpShared

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 808245
User impact if declined: Unlikely crashes
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

Comment 9

[bhavana bajaj [:bajaj]]

Comment on attachment 698851 [details] [diff] [review]
Root RegExp source for the lifetime of RegExpShared

Low risk patch for a Fx 20 sec-crit regression.Considering where we are in the cycle this is manageable risk, hence approving it on aurora.

Comment 10

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/62b19c4e8696

Comment 11

[Sean Stangl [:sstangl]]

Debugging Bug 820124 revealed that a small issue exists in this patch, which can lead to random crashes of a similar variety to the original bug. The HeapPtr must be a raw pointer.

Comment 12

[Sean Stangl [:sstangl]]

Attachment: follow-up fix

Comment 13

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 44dcffe8792b).

Comment 15

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0dbf0686ae

Will also need landing on aurora after it hits m-c. I'll assume that the a=bajaj from above is valid for this fix, and land it tomorrow.

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Will also need landing on aurora after it hits m-c. I'll assume that the
> a=bajaj from above is valid for this fix, and land it tomorrow.

Request for approval for aurora landing will have to be made again because this is a separate patch. :|

Comment 17

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/mozilla-central/rev/fb0dbf0686ae

Comment 18

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

[Sean Stangl [:sstangl]]

Comment on attachment 701381 [details] [diff] [review]
follow-up fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 826581 (previous patch)
User impact if declined: 

The first patch in this bug, which fixed the issue originally identified, introduced an extremely subtle bug involving prebarriers called during destructors. This can manifest as extremely unlikely crashes that are difficult to debug and obscure as to their cause.

Testing completed (on m-c, etc.): m-c and significant fuzzing as part of Bug 820124, which identified the issue.

Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

Comment 20

[Ryan VanderMeulen [:RyanVM]]

I've got this in my queue for Aurora landing once it reopens. Clearing checkin-needed so this doesn't show up in my queries.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2aeb8194eb23