Closed Bug 791174 Opened 12 years ago Closed 12 years ago

Object with freed shape causes JSObject::isFunction() to crash with deterministicgc, gcslice


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox16 --- unaffected
firefox17 + fixed
firefox18 + verified
firefox19 + verified
firefox-esr10 --- unaffected


(Reporter: jruderman, Assigned: billm)



(5 keywords, Whiteboard: [adv-track-main17-])

Crash Data


(3 files)

Attached file testcase
1. Install
2. Load the testcase (might have to be loaded from the command line)

Result: crash
Attached file stack trace
Crash Signature: [@ js::Shape::getObjectClass]
This isn't the memory reporter's fault.  It just calls JSObject::isFunction() on all objects;  the problem is with the object itself.

Specifically, the object's shape has been freed:

(gdb) p *obj->lastProperty()
$10 = {<js::gc::Cell> = {static CellShift = 3, static CellSize = 8, 
    static CellMask = 7}, 
  base_ = {<js::EncapsulatedPtr<js::BaseShape, unsigned long>> = {{
        value = 0xdadadadadadadada, 
        other = 15770157678700714714}}, <No data fields>}, propid_ = {
    value = {asBits = 15770157678700714714}}, slotInfo = 3671775962, 
  attrs = 218 '\332', flags = 218 '\332', shortid_ = -9510, 
  parent = {<js::EncapsulatedPtr<js::Shape, unsigned long>> = {{
        value = 0xdadadadadadadada, 
        other = 15770157678700714714}}, <No data fields>}, {kids = {
      w = 15770157678700714714}, listp = 0xdadadadadadadada}}

Look at all those 0xda values!
OS: Mac OS X → All
Hardware: x86_64 → All
Summary: about:memory crash with deterministicgc, gcslice → Object with freed shape causes JSObject::isFunction() to crash with deterministicgc, gcslice
Group: core-security
Nick, did you do anything special to reproduce this? I tried it a week ago and it didn't crash.
David, could this be related to the IonMonkey memory corruption issue?
This doesn't look like the typical stack from that bug.
Assignee: general → wmccloskey
Attached patch fixSplinter Review
I was able to reproduce this with Jesse's help. This is a regression from bug 729760, so it affects 17 and up.

The basic sequence is as follows:
1. The test starts an incremental GC, which runs until we're part way through incremental sweeping.
2. There's an unmarked background-finalizable object O that points to shape S. Part way through incremental sweeping, S will get swept.
3. Before the GC finishes and background finalization starts, the test opens about:memory. This causes us to call IterateCompartmentsArenasCells. It sees the object O and tries to get its shape, which has been swept. We crash there.

I think we actually considered this situation for bug 729760, but we thought that it was okay to allow the iterator to see the unmarked object, since nothing bad had been done to it yet. However, I forgot that it could point to things that had been swept.

Anyway, I think the best fix is to finish any ongoing incremental GC before doing any heap iteration. I also cleaned up the code we use for a number of these iteration/tracing routines.

One note about TraceRuntime: it used to be written in an awkward way to allow us to call it even when a GC was in progress. This was so that you could call JS_DumpHeap from within a GC during debugging. If we actually need to do this, though, I think it would be much easier to comment out the assertion that fires when you try to do this. I've only called JS_DumpHeap from within a GC one or two times, ever.
Attachment #670585 - Flags: review?(jcoppeard)
Comment on attachment 670585 [details] [diff] [review]

Review of attachment 670585 [details] [diff] [review]:

This sounds like the safest and simplest thing to do.  The patch looks good.
Attachment #670585 - Flags: review?(jcoppeard) → review+
Comment on attachment 670585 [details] [diff] [review]

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Not very easily. I considered this very problem earlier and thought it was harmless.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?
17 and up.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
It will be easy to port to aurora and beta.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely to regress anything.
Attachment #670585 - Flags: sec-approval?
Comment on attachment 670585 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 729760
User impact if declined: Random crashes when telemetry ping happens.
Testing completed (on m-c, etc.): Will land on m-c first.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.
Attachment #670585 - Flags: approval-mozilla-beta?
Attachment #670585 - Flags: approval-mozilla-aurora?
Attachment #670585 - Flags: sec-approval? → sec-approval+
Attachment #670585 - Flags: approval-mozilla-beta?
Attachment #670585 - Flags: approval-mozilla-beta+
Attachment #670585 - Flags: approval-mozilla-aurora?
Attachment #670585 - Flags: approval-mozilla-aurora+
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
I guess I used a function that doesn't exist in beta. Fixed:
Keywords: verifyme
Whiteboard: [adv-track-main17-]
Group: core-security
Keywords: regression
Verified the crash for the 2012-09-13 Nightly Debug following the STR from comment 0.

No crashes for Firefox Debug 19.0 beta 2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:19.0) Gecko/20130119 Firefox/19.0
Build ID:  20130119165213

and Firefox Debug 18.0.1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:18.0) Gecko/20130120 Firefox/18.0
Build ID: 20130120043114
Is it possible/does it make sense to create and commit a test here?
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.