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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Load the testcase (might have to be loaded from the command line)

Result: crash

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Nicholas Nethercote [inactive]]

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!

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Nick, did you do anything special to reproduce this? I tried it a week ago and it didn't crash.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

David, could this be related to the IonMonkey memory corruption issue?

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

This doesn't look like the typical stack from that bug.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix

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.

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 670585 [details] [diff] [review]
fix

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

This sounds like the safest and simplest thing to do.  The patch looks good.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 670585 [details] [diff] [review]
fix

[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?
No.

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.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 670585 [details] [diff] [review]
fix

[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.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Backed out for test failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a4f26cbb78

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70016bc36b45

Comment 14

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/70016bc36b45

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9cd19c6cf250
https://hg.mozilla.org/releases/mozilla-beta/rev/db565b9f4d63

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I guess I used a function that doesn't exist in beta. Fixed:
https://hg.mozilla.org/releases/mozilla-beta/rev/42f2d547549e

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Almost there:
https://hg.mozilla.org/releases/mozilla-beta/rev/58efa156cb7e

Comment 18

[Virgil Dicu [:virgil] [QA]]

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

Comment 19

[Christian Holler (:decoder)]

Is it possible/does it make sense to create and commit a test here?

Comment 20

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old