Assertion failure: [barrier verifier] Unmarked edge: object slot, at js/src/gc/Verifier.cpp:301

VERIFIED FIXED in Firefox 47

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
a year ago
9 months ago

People

(Reporter: gkw, Assigned: jonco)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla49
x86_64
Mac OS X
assertion, csectype-uaf, sec-high, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 wontfix, firefox46 wontfix, firefox47+ fixed, firefox48+ verified, firefox49 verified, firefox-esr38 wontfix, firefox-esr4547+ fixed)

Details

(Whiteboard: [fuzzblocker] [jsbugmon:update][adv-main47+][adv-esr45.2+])

Attachments

(2 attachments)

(Reporter)

Description

a year ago
The following testcase crashes on mozilla-central revision 8630367f5e3f (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

// Adapted from randomly chosen test: js/src/jit-test/tests/arguments/defaults-destructuring-mixed-default-value.js
function f(x, [y]) {}
f(0, []);
// jsfunfuzz-generated
for (var z of [0, 0, 0]) {
    verifyprebarriers();
}

Backtrace:

0   js-dbg-64-dm-clang-darwin-8630367f5e3f	0x0000000100a3dd07 js::gc::GCRuntime::endVerifyPreBarriers() + 1175 (Verifier.cpp:302)
1   js-dbg-64-dm-clang-darwin-8630367f5e3f	0x00000001005f7132 js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) + 210 (GCInternals.h:88)
2   js-dbg-64-dm-clang-darwin-8630367f5e3f	0x00000001005e6756 js::gc::GCRuntime::gc(JSGCInvocationKind, JS::gcreason::Reason) + 86 (jsgc.cpp:6536)
3   js-dbg-64-dm-clang-darwin-8630367f5e3f	0x0000000100588d8a js::DestroyContext(JSContext*, js::DestroyContextMode) + 458 (Utility.h:400)
4   js-dbg-64-dm-clang-darwin-8630367f5e3f	0x00000001000069cc main + 12396 (js.cpp:7467)
5   js-dbg-64-dm-clang-darwin-8630367f5e3f	0x00000001000019a4 start + 52

Setting s-s because gc seems to be involved.
(Reporter)

Comment 1

a year ago
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/f88bfa306282
user:        Greg Weng
date:        Tue Apr 12 18:29:00 2016 +0200
summary:     Bug 1198701 - ArrayIterator gets length property after iteration has finished. r=till

Greg, is bug 1198701 a likely regressor?
Blocks: 1198701
Flags: needinfo?(gweng)
(Reporter)

Comment 2

a year ago
Happens fairly often -> [fuzzblocker]
Whiteboard: [jsbugmon:update] → [fuzzblocker][jsbugmon:update]
I attempted to change the line setting the whole internal slot as NULL, but so far there is no luck even I set it as an special object not a NULL. So I think the root case looks like I change the shape of the object that something is expecting. I may do some more experiments to change no shape and then test it again.
Flags: needinfo?(gweng)
I've found that if I set a special key to the existing object of ITERATOR_SLOT_TARGET, rather than set it as a new object (no matter whether it's a NULL or not), it will pass the fuzzy test. However, this is conflicted to the spec, which requires to set the slot as "undefined", and I still haven't got the clue of exactly why.
My latest test prove that once I set the ITERATOR_SLOT_TARGET as any other new object, like to create a self-hosted function:

    function TestBarrier() {
        UnsafeSetReservedSlot(this, ITERATOR_SLOT_TARGET, {something: 'not null'});
    }

And then binding it on the iterator:

    static const JSFunctionSpec array_iterator_methods[] = {
        JS_SELF_HOSTED_FN("next", "ArrayIteratorNext", 0, 0),
        JS_SELF_HOSTED_FN("testBarrier", "TestBarrier", 0, 0),
        JS_FS_END
    };

Then execute the test in JSShell:

    var a = [1,2,3];
    var i = a.values();    // get the iterator
    i.testBarrier();
    verifyprebarriers()

It would failed as the original case in this bug, although it's not always reproducible, as the original one.

As a result, I think the issue is for some reasons currently unknown to me, if I set the ITERATOR_SLOT_TARGET as any object (or, I suspect it could be anything) instead of the built one, it would fail to pass the verification.
For the Bug 1198701 I think the bad news is the workaround is to modify the ITERATOR_SLOT_TARGET, rather than set it as undefined or null as the spec suggests. However, I still need to dig in that to see what the thing is, and how to add a field to indicate the it has been "nullified" although it's actually not.

Another choice is I need to there are other Self-hosted APIs or methods I can use to fix the issue. After all, my patch is a pure JavaScript patch, if I can fix it with just additional JavaScript changes that would be great. However, the issue is I don't know if there are such modifications exist.

The worst case is I need to do some C++ modification related to GC to make sure the case works well. However, since I'm new to these GC things, and it's not 100% reproducible, I will need help and may take a long time to debug this.
(Assignee)

Comment 7

a year ago
Created attachment 8742851 [details] [diff] [review]
bug1264575-pre-barrier

We're missing a pre-barrier when we inline the UnsafeSetReservedSlot intrinsic.
Assignee: nobody → jcoppeard
Attachment #8742851 - Flags: review?(jdemooij)
Attachment #8742851 - Flags: review?(jdemooij) → review+
Keywords: csectype-uaf, sec-high
(Assignee)

Updated

a year ago
Blocks: 619558
(Assignee)

Updated

a year ago
Blocks: 844882
No longer blocks: 619558
(Assignee)

Comment 8

a year ago
Comment on attachment 8742851 [details] [diff] [review]
bug1264575-pre-barrier

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Hard, would require control of incremental GC timing.

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? Everything back to FF24.

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

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

How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions.
Attachment #8742851 - Flags: sec-approval?
This has sec-approval for checkin to trunk on May 10, two weeks into the next cycle (we're releasing Firefox 46 this coming Tuesday). 

We'll want Aurora, Beta, and ESR45 patches to go in following trunk, I expect. You may wish to create and nominate them now.
status-firefox45: --- → wontfix
status-firefox46: --- → wontfix
status-firefox47: --- → affected
status-firefox-esr38: --- → wontfix
status-firefox-esr45: --- → affected
tracking-firefox47: --- → +
tracking-firefox48: --- → +
tracking-firefox-esr45: --- → ?
Attachment #8742851 - Flags: sec-approval? → sec-approval+
Whiteboard: [fuzzblocker][jsbugmon:update] → [fuzzblocker][jsbugmon:update][checkin on 5/10]
(Assignee)

Comment 10

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/55aa8c623d4133eccc000cfa61f32c0809d97330

Updated

a year ago
Whiteboard: [fuzzblocker][jsbugmon:update][checkin on 5/10] → [fuzzblocker] [checkin on 5/10] [jsbugmon:update,ignore]

Comment 11

a year ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision c3f5e6079284).
https://hg.mozilla.org/mozilla-central/rev/55aa8c623d41
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Reporter)

Updated

a year ago
Whiteboard: [fuzzblocker] [checkin on 5/10] [jsbugmon:update,ignore] → [fuzzblocker] [jsbugmon:update]

Updated

a year ago
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified

Comment 13

a year ago
JSBugMon: This bug has been automatically verified fixed.
Hello Jon, could you please nominate patches for uplift to Aurora, Beta, ESR45? Thanks!
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 15

a year ago
Comment on attachment 8742851 [details] [diff] [review]
bug1264575-pre-barrier

Approval Request Comment
[Feature/regressing bug #]: Bug 844882.
[User impact if declined]: Possible crashes / vulnerability.
[Describe test coverage new/current, TreeHerder]: On m-c since 12th May.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8742851 - Flags: approval-mozilla-beta?
Attachment #8742851 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 16

a year ago
Created attachment 8752744 [details] [diff] [review]
bug1264575-pre-barrier-esr45

Approval Request Comment
User impact if declined: Possible crashes / vulnerability.
Fix Landed on Version: 49
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None
Attachment #8752744 - Flags: review+
Attachment #8752744 - Flags: approval-mozilla-esr45?
Comment on attachment 8742851 [details] [diff] [review]
bug1264575-pre-barrier

sec-high, Aurora48+, Beta47+
Attachment #8742851 - Flags: approval-mozilla-beta?
Attachment #8742851 - Flags: approval-mozilla-beta+
Attachment #8742851 - Flags: approval-mozilla-aurora?
Attachment #8742851 - Flags: approval-mozilla-aurora+

Updated

a year ago
Attachment #8752744 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+

Updated

a year ago
tracking-firefox-esr45: ? → 47+
https://hg.mozilla.org/releases/mozilla-aurora/rev/df9b55a0fed6
https://hg.mozilla.org/releases/mozilla-beta/rev/83e74e1d0cd1
https://hg.mozilla.org/releases/mozilla-esr45/rev/09418166fd77ea4f634d3db94e631a4a62998999
status-firefox47: affected → fixed
status-firefox48: affected → fixed
status-firefox-esr45: affected → fixed
Group: javascript-core-security → core-security-release

Updated

a year ago
status-firefox48: fixed → verified

Comment 19

a year ago
JSBugMon: This bug has been automatically verified fixed on Fx48
Whiteboard: [fuzzblocker] [jsbugmon:update] → [fuzzblocker] [jsbugmon:update][adv-main47+][adv-esr45.2+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.