Closed Bug 1264575 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 + fixed
firefox48 + verified
firefox49 --- verified
firefox-esr38 --- wontfix
firefox-esr45 47+ fixed

People

(Reporter: gkw, Assigned: jonco)

References

(Blocks 2 open bugs)

Details

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

Attachments

(2 files)

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.
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)
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.
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+
Blocks: 844882
No longer blocks: GenerationalGC
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.
Attachment #8742851 - Flags: sec-approval? → sec-approval+
Whiteboard: [fuzzblocker][jsbugmon:update] → [fuzzblocker][jsbugmon:update][checkin on 5/10]
Whiteboard: [fuzzblocker][jsbugmon:update][checkin on 5/10] → [fuzzblocker] [checkin on 5/10] [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision c3f5e6079284).
https://hg.mozilla.org/mozilla-central/rev/55aa8c623d41
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Whiteboard: [fuzzblocker] [checkin on 5/10] [jsbugmon:update,ignore] → [fuzzblocker] [jsbugmon:update]
Status: RESOLVED → VERIFIED
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)
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?
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+
Attachment #8752744 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Group: javascript-core-security → core-security-release
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.