Crash [@ JSObject::finalize] or [@ js::gc::GCRuntime::sweepBackgroundThings]

VERIFIED FIXED in Firefox 38

Status

()

--
critical
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: gkw, Assigned: jonco)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla40
x86_64
macOS
crash, regression, sec-moderate, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox37 unaffected, firefox38 fixed, firefox39 verified, firefox40 verified, firefox-esr31 unaffected, firefox-esr38 fixed, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-master fixed)

Details

(crash signature)

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
// Randomly chosen test: js/src/jit-test/tests/debug/Memory-onGarbageCollection-02.js
dbg1 = new Debugger();
root2 = newGlobal();
dbg1.memory.onGarbageCollection = function(){}
dbg1.addDebuggee(root2);
// jsfunfuzz-generated code
for (var j = 0; j < 9999; ++j) {
    try {
        a
    } catch (e) {}
}
// Randomly chosen test: js/src/jit-test/tests/basic/testBug756919.js
gcparam("maxBytes", gcparam("gcBytes") + 1);
g();
function g() {
    var x = "";
    function f() {}
    eval('');
    g();
}

crashes js 32-bit debug and opt shell on m-c changeset cbd0efcd976c with --fuzzing-safe --no-threads --no-baseline --no-ion at JSObject::finalize with js::gc::GCRuntime::sweepBackgroundThings on the stack.

Debug configure options:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build --32" -r cbd0efcd976c

Opt configure options:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --disable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--disable-debug --enable-more-deterministic --enable-nspr-build --32" -r cbd0efcd976c

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/c109ca33d8a7
user:        Jon Coppeard
date:        Tue Mar 17 16:07:40 2015 +0000
summary:     Bug 1144108 - Fix debugger tests that are confused by GC zeal r=terrence

Setting s-s to be safe, not sure if a weird memory address is being accessed here, this might be an OOM bug.

Jon, is bug 1144108 a likely regressor?
Flags: needinfo?(jcoppeard)
(Reporter)

Comment 1

4 years ago
Posted file stack of opt crash
(lldb) bt 5
* thread #1: tid = 0x80832, 0x005383d2 js-32-dm-nsprBuild-darwin-cbd0efcd976c`unsigned long js::gc::Arena::finalize<JSObject>(js::FreeOp*, js::gc::AllocKind, unsigned long) [inlined] js::ObjectGroup::clasp(this=0x4b4b4b4b) const at ObjectGroup.h:199, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x4b4b4b4b)
  * frame #0: 0x005383d2 js-32-dm-nsprBuild-darwin-cbd0efcd976c`unsigned long js::gc::Arena::finalize<JSObject>(js::FreeOp*, js::gc::AllocKind, unsigned long) [inlined] js::ObjectGroup::clasp(this=0x4b4b4b4b) const at ObjectGroup.h:199
    frame #1: 0x005383d2 js-32-dm-nsprBuild-darwin-cbd0efcd976c`unsigned long js::gc::Arena::finalize<JSObject>(js::FreeOp*, js::gc::AllocKind, unsigned long) [inlined] JSObject::getClass() const + 3 at jsobj.h:128
    frame #2: 0x005383cf js-32-dm-nsprBuild-darwin-cbd0efcd976c`unsigned long js::gc::Arena::finalize<JSObject>(js::FreeOp*, js::gc::AllocKind, unsigned long) [inlined] JSObject::finalize(fop=<unavailable>) at jsobjinlines.h:59
    frame #3: 0x005383cf js-32-dm-nsprBuild-darwin-cbd0efcd976c`unsigned long js::gc::Arena::finalize<JSObject>(this=<unavailable>, fop=0x0004f300, thingKind=OBJECT8_BACKGROUND, thingSize=<unavailable>) + 415 at jsgc.cpp:500
    frame #4: 0x004dae81 js-32-dm-nsprBuild-darwin-cbd0efcd976c`FinalizeArenas(js::FreeOp*, js::gc::ArenaHeader**, js::gc::SortedArenaList&, js::gc::AllocKind, js::SliceBudget&, js::gc::ArenaLists::KeepArenasEnum) [inlined] bool FinalizeTypedArenas<JSObject>(src=0xbfffe028, keepArenas=<unavailable>) + 219 at jsgc.cpp:560
(lldb)
(Reporter)

Comment 2

4 years ago
Posted file debug stack
(lldb) bt 5
* thread #1: tid = 0x804da, 0x0089ce26 js-dbg-32-dm-nsprBuild-darwin-cbd0efcd976c`JSObject::finalize(js::FreeOp*) [inlined] js::ObjectGroup::clasp(this=0x4b4b4b4b) const at ObjectGroup.h:199, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x4b4b4b4b)
  * frame #0: 0x0089ce26 js-dbg-32-dm-nsprBuild-darwin-cbd0efcd976c`JSObject::finalize(js::FreeOp*) [inlined] js::ObjectGroup::clasp(this=0x4b4b4b4b) const at ObjectGroup.h:199
    frame #1: 0x0089ce26 js-dbg-32-dm-nsprBuild-darwin-cbd0efcd976c`JSObject::finalize(js::FreeOp*) [inlined] JSObject::getClass(this=0x0411b060) const + 2 at jsobj.h:128
    frame #2: 0x0089ce24 js-dbg-32-dm-nsprBuild-darwin-cbd0efcd976c`JSObject::finalize(this=0x0411b060, fop=0xbfffdfb0) + 116 at jsobjinlines.h:59
    frame #3: 0x0089c5a2 js-dbg-32-dm-nsprBuild-darwin-cbd0efcd976c`unsigned long js::gc::Arena::finalize<JSObject>(this=<unavailable>, fop=<unavailable>, thingKind=<unavailable>, thingSize=<unavailable>) + 450 at jsgc.cpp:500
    frame #4: 0x007f8e1c js-dbg-32-dm-nsprBuild-darwin-cbd0efcd976c`FinalizeArenas(js::FreeOp*, js::gc::ArenaHeader**, js::gc::SortedArenaList&, js::gc::AllocKind, js::SliceBudget&, js::gc::ArenaLists::KeepArenasEnum) [inlined] bool FinalizeTypedArenas<JSObject>(src=0xbfffdf68, dest=<unavailable>) + 220 at jsgc.cpp:560
(lldb)
(Assignee)

Updated

4 years ago
Assignee: nobody → jcoppeard
Crash Signature: [@ JSObject::finalize] [@ js::gc::GCRuntime::sweepBackgroundThings] → [@ JSObject::finalize] [@ js::gc::GCRuntime::sweepBackgroundThings]
Keywords: sec-high
(Assignee)

Comment 3

4 years ago
It looks like the free span for an arena allocated while inside the debugger onGarbageCollection hook is not being correctly update.  When we come to sweeping we try to finalize a cell that has not been initialized.  I can't see how this is happening however.

Changin the onGarbageCollection hook to use a runnable rather than being called directly by the GC would likely fix this problem.
Flags: needinfo?(jcoppeard)
Lowering to moderate because it requires the debugger do something.
Keywords: sec-high → sec-moderate
(In reply to Jon Coppeard (:jonco) from comment #3)
> Changin the onGarbageCollection hook to use a runnable rather than being
> called directly by the GC would likely fix this problem.

Note: it will likely be a bit before I get around to this b/c it isn't part of my Q1 goals.
(Assignee)

Comment 6

4 years ago
What's happening is that a last ditch GC is triggered (due to setting maxBytes) and after that we are able to allocate a new arena.  But the debugger callback that ran at the end of GC also allocated a new new arena for the required kind, and GCRuntime::refillFreeListFromMainThread() ignores that possibility, overwriting the free list with the span of another arena.  So we lose the fact that any of the original arena's cells were not allocated, and when we sweep we try and finalize them all.

This is only a problem if any of the GC callbacks are allowed to allocate GC things.  At the moment this debugger callback definitely does, but this might be possible for the cycle collector ones too.
(Assignee)

Comment 7

4 years ago
This is fallout from bug 1130475.
Blocks: 1130475
No longer blocks: 1144108
(Assignee)

Comment 8

4 years ago
Attachment #8583145 - Flags: review?(terrence)
(In reply to Jon Coppeard (:jonco) from comment #6)
> This is only a problem if any of the GC callbacks are allowed to allocate GC
> things.  At the moment this debugger callback definitely does, but this
> might be possible for the cycle collector ones too.

I don't think it does, though there are a few logging things that are not on by default that might possibly do that.
status-firefox37: --- → unaffected
status-firefox38: --- → affected
Comment on attachment 8583145 [details] [diff] [review]
last-ditch-fix

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

Ouch!
Attachment #8583145 - Flags: review?(terrence) → review+
(Assignee)

Comment 12

4 years ago
Fixed testcase not to fail with too much recursion on opt builds:

https://hg.mozilla.org/integration/mozilla-inbound/rev/49aa36d0b28a
(Assignee)

Comment 13

4 years ago
And backed out again for continued presence of jit-test failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/aef75ff8a911
(Assignee)

Comment 15

4 years ago
And more test bustage, this time on Windows.

https://hg.mozilla.org/integration/mozilla-inbound/rev/490d2e6a5026
Only took 3 attempts, but we finally found the actual culprits for the leaks in comment 16. Wasn't this bug. Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9137f699a08
Flags: in-testsuite+
(Assignee)

Comment 19

4 years ago
(In reply to Wes Kocher (:KWierso) from comment #18)
> now linux cgc tests are permafailing:

Right, I think that's due to bug 1145997.  Looks like we'll need to fix that first.
Flags: needinfo?(jcoppeard)
https://hg.mozilla.org/mozilla-central/rev/2efcea316e96

Please nominate this for Aurora/Beta approval when you get a chance.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-master: --- → fixed
status-firefox40: --- → fixed
status-firefox-esr31: --- → unaffected
status-firefox-esr38: --- → affected
Flags: needinfo?(jcoppeard)
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 22

4 years ago
Comment on attachment 8583145 [details] [diff] [review]
last-ditch-fix

Approval Request Comment
[Feature/regressing bug #]: Bug 1130475.
[User impact if declined]: Possible crash in low memory conditions.
[Describe test coverage new/current, TreeHerder]: On central since yesterday.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8583145 - Flags: approval-mozilla-beta?
Attachment #8583145 - Flags: approval-mozilla-aurora?
Comment on attachment 8583145 [details] [diff] [review]
last-ditch-fix

Should be in 38 beta 2
Attachment #8583145 - Flags: approval-mozilla-beta?
Attachment #8583145 - Flags: approval-mozilla-beta+
Attachment #8583145 - Flags: approval-mozilla-aurora?
Attachment #8583145 - Flags: approval-mozilla-aurora+
Needs rebasing for beta.
Flags: needinfo?(jcoppeard)
Status: RESOLVED → VERIFIED
Crash Signature: [@ JSObject::finalize] [@ js::gc::GCRuntime::sweepBackgroundThings] → [@ JSObject::finalize] [@ js::gc::GCRuntime::sweepBackgroundThings]
status-firefox39: fixed → verified
status-firefox40: fixed → verified
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx39
Looks like Jon landed this on beta.
https://hg.mozilla.org/releases/mozilla-beta/rev/484a6aef6a4f
Crash Signature: [@ JSObject::finalize] [@ js::gc::GCRuntime::sweepBackgroundThings] → [@ JSObject::finalize] [@ js::gc::GCRuntime::sweepBackgroundThings]
status-firefox38: affected → fixed
status-firefox-esr38: affected → fixed
Flags: needinfo?(jcoppeard)

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.