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

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

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

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.

Comment 2

[Jon Coppeard (:jonco)]

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

Comment 3

[Jon Coppeard (:jonco)]

Attachment: backtrace.txt

Successfully reproduced with Gary's testcase and a local debug shell build.  Here's a backtrace.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1116306-drainAllocationsLog

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.

Comment 5

[Al Billings [:abillings - ex-MoCo]]

How far back does this issue go? Is ESR31 affected?

Comment 6

[Steve Fink [:sfink] [:s:]]

(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 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8542559 [details] [diff] [review]
bug1116306-drainAllocationsLog

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

Nice analysis.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

(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 9

[Jon Coppeard (:jonco)]

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.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 11

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f85d78f6890

Comment 12

[Ryan VanderMeulen [:RyanVM]]

This wasn't supposed to land until 1/26?

Comment 13

[Andrew McCreight [:mccr8]]

This looks like a crash involving the GC and the debugger, so sec-critical seems too high.  sec-moderate might even be more appropriate.

Comment 14

[Daniel Veditz [:dveditz]]

If this requires a debug-build then I agree with sec-moderate.

Comment 15

[Andrew McCreight [:mccr8]]

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

Comment 16

[Andrew McCreight [:mccr8]]

This is probably not worth backporting, but feel free to nominate and set this back to affected for 36 if you disagree, Jon.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8f85d78f6890

Comment 19

[Jon Coppeard (:jonco)]

(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 :(