Assertion failure: [barrier verifier] Unmarked edge: allocation log SavedFrame, at gc/Verifier.cpp

RESOLVED FIXED in Firefox 37

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: gkw, Assigned: jonco)

Tracking

(Blocks 2 bugs, 4 keywords)

Trunk
mozilla37
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 unaffected, firefox35 wontfix, firefox36 wontfix, firefox37+ fixed, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main37+])

Attachments

(2 attachments)

The upcoming testcase asserts js debug shell on m-c changeset 6d5a1e68f248 with --fuzzing-safe --gc-zeal=14 --no-threads --no-baseline --ion-eager at Assertion failure: [barrier verifier] Unmarked edge: allocation log SavedFrame, at gc/Verifier.cpp

The shell was obtained from:

https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-12-29-mozilla-central-debug/

in particular:

https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-12-29-mozilla-central-debug/jsshell-mac64.zip

This was found by combining random js tests together with jsfunfuzz, the specific file(s) is/are:

http://hg.mozilla.org/mozilla-central/file/6d5a1e68f248/js/src/jit-test/tests/basic/bug749039.js
http://hg.mozilla.org/mozilla-central/file/6d5a1e68f248/js/src/tests/test262/ch08/8.12/8.12.1/8.12.1-1_8.js
http://hg.mozilla.org/mozilla-central/file/6d5a1e68f248/js/src/jit-test/tests/debug/Memory-drainAllocationsLog-09.js

Setting s-s since this involves gc stuff and the barrier, assuming sec-critical but please feel free to change this as necessary.

Bisection is not reliable - I cannot reproduce on a local testcase, and so I do not have a reliable stack.
Flags: needinfo?(terrence)
Flags: needinfo?(sphink)
Flags: needinfo?(jcoppeard)
Gary, do you have a specific test case for this?  I concatenated the three files referenced above but could not reproduce this using the shell linked.

The edge we're asserting on is a debugger allocation log pointer to a frame:

http://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#2263

This is a RelocatablePtrObject though so this error from the verifier shouldn't happen:

http://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.h#254
Flags: needinfo?(jcoppeard) → needinfo?(gary)
Posted file backtrace.txt
Successfully reproduced with Gary's testcase and a local debug shell build.  Here's a backtrace.
The problem is in DebuggerMemory::drainAllocationsLog():

http://dxr.mozilla.org/mozilla-central/source/js/src/vm/DebuggerMemory.cpp#200

This removes the front element of the allocation log list, taking care to wrap it in a UniquePtr so that it is deleted when we leave the scope.  However this is not allowed because it alters the object graph without invoking the necessary pre-barriers, and breaks the snapshot-at-the-beginning invariant we have that ensures incremental GC works correctly.

If we perform and incremental GC slice immediately after this we could end up deleting the still-referenced frame object.  It seems that rooting the frame would prevent this, but roots are only marked in the first slice of an incremental GC.

The fix is to leave the element on the list until we delete it, which invokes the pre-barriers.
Assignee: nobody → jcoppeard
Flags: needinfo?(terrence)
Flags: needinfo?(sphink)
Flags: needinfo?(gary)
Attachment #8542559 - Flags: review?(sphink)
How far back does this issue go? Is ESR31 affected?
(In reply to Al Billings [:abillings] from comment #5)
> How far back does this issue go? Is ESR31 affected?

No, it's from bug 1066313, which has a milestone of Fx 35.
Comment on attachment 8542559 [details] [diff] [review]
bug1116306-drainAllocationsLog

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

Nice analysis.
Attachment #8542559 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink, :s:] from comment #6)
> (In reply to Al Billings [:abillings] from comment #5)
> > How far back does this issue go? Is ESR31 affected?
> 
> No, it's from bug 1066313, which has a milestone of Fx 35.

Ah. Unfortunately, we're too late to get this fixed in 35 as we have no betas left and ship in two weeks. 

If this might cause a chemspill, we could fight to take it in 35. Otherwise, it is out.
Comment on attachment 8542559 [details] [diff] [review]
bug1116306-drainAllocationsLog

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Difficult as it would involve getting an incremental GC slice to happen at exactly the right time.

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

Not really.

Which older supported branches are affected by this flaw? 

FF 35 to present.

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

Bug 1066313.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The patch should apply cleanly to all affected branches.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely.
Attachment #8542559 - Flags: sec-approval?
This is too late to make 35, which has no betas left and is having its release candidate built on Monday. This is sec-approval+ for checkin on 1/26, two weeks into the next cycle.
Whiteboard: [checkin on 1/26]
Attachment #8542559 - Flags: sec-approval? → sec-approval+
This wasn't supposed to land until 1/26?
This looks like a crash involving the GC and the debugger, so sec-critical seems too high.  sec-moderate might even be more appropriate.
Keywords: sec-criticalsec-high
Whiteboard: [checkin on 1/26]
If this requires a debug-build then I agree with sec-moderate.
Keywords: sec-highsec-moderate
(In reply to Daniel Veditz [:dveditz] from comment #14)
> If this requires a debug-build then I agree with sec-moderate.

Not a debug build, the JS debugger.  But yeah, I'd be surprised if you could actually exploit this.
This is probably not worth backporting, but feel free to nominate and set this back to affected for 36 if you disagree, Jon.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #12)
> This wasn't supposed to land until 1/26?

That was correct. 

Dan just re-rated it to a sec-moderate, which means it no longer needs sec-approval+ to land on trunk but, as it was, this should not have gone in.
https://hg.mozilla.org/mozilla-central/rev/8f85d78f6890
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(In reply to Al Billings [:abillings] from comment #17)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #12)
> > This wasn't supposed to land until 1/26?
> That was correct.

Guys, I'm sorry this was a reading comprehension fail on my part :(
Group: javascript-core-security
Whiteboard: [adv-main37+]
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.