Closed
Bug 1264575
Opened 8 years ago
Closed 8 years ago
Assertion failure: [barrier verifier] Unmarked edge: object slot, at js/src/gc/Verifier.cpp:301
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
People
(Reporter: gkw, Assigned: jonco)
References
Details
(4 keywords, Whiteboard: [fuzzblocker] [jsbugmon:update][adv-main47+][adv-esr45.2+])
Attachments
(2 files)
1.43 KB,
patch
|
jandem
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
1.57 KB,
patch
|
jonco
:
review+
ritu
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
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•8 years 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•8 years ago
|
||
Happens fairly often -> [fuzzblocker]
Whiteboard: [jsbugmon:update] → [fuzzblocker][jsbugmon:update]
Comment 3•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
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.
Comment 5•8 years ago
|
||
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.
Comment 6•8 years ago
|
||
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•8 years ago
|
||
We're missing a pre-barrier when we inline the UnsafeSetReservedSlot intrinsic.
Assignee: nobody → jcoppeard
Attachment #8742851 -
Flags: review?(jdemooij)
Updated•8 years ago
|
Attachment #8742851 -
Flags: review?(jdemooij) → review+
Updated•8 years ago
|
Keywords: csectype-uaf,
sec-high
Assignee | ||
Updated•8 years ago
|
Blocks: GenerationalGC
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 8•8 years 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?
Comment 9•8 years ago
|
||
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:
--- → ?
Updated•8 years ago
|
Attachment #8742851 -
Flags: sec-approval? → sec-approval+
Updated•8 years ago
|
Whiteboard: [fuzzblocker][jsbugmon:update] → [fuzzblocker][jsbugmon:update][checkin on 5/10]
Assignee | ||
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/55aa8c623d4133eccc000cfa61f32c0809d97330
Updated•8 years ago
|
Whiteboard: [fuzzblocker][jsbugmon:update][checkin on 5/10] → [fuzzblocker] [checkin on 5/10] [jsbugmon:update,ignore]
Comment 11•8 years ago
|
||
JSBugMon: The testcase found in this bug no longer reproduces (tried revision c3f5e6079284).
Comment 12•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/55aa8c623d41
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Reporter | ||
Updated•8 years ago
|
Whiteboard: [fuzzblocker] [checkin on 5/10] [jsbugmon:update,ignore] → [fuzzblocker] [jsbugmon:update]
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Comment 13•8 years 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•8 years 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•8 years ago
|
||
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+
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
Updated•8 years ago
|
Group: javascript-core-security → core-security-release
Updated•8 years ago
|
Comment 19•8 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx48
Updated•8 years ago
|
Whiteboard: [fuzzblocker] [jsbugmon:update] → [fuzzblocker] [jsbugmon:update][adv-main47+][adv-esr45.2+]
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•