Closed
Bug 1138391
Opened 8 years ago
Closed 8 years ago
Crash [@ js::jit::JitFrameIterator::checkInvalidation] or Crash [@ js::jit::SnapshotIterator::SnapshotIterator] with weird address
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox36 | --- | wontfix |
firefox37 | + | fixed |
firefox38 | + | verified |
firefox39 | + | verified |
firefox-esr31 | --- | unaffected |
b2g-v1.4 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | fixed |
b2g-master | --- | fixed |
People
(Reporter: decoder, Assigned: nbp)
References
Details
(4 keywords, Whiteboard: [jsbugmon:][adv-main37+])
Crash Data
Attachments
(3 files)
10.30 KB,
patch
|
h4writer
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
9.96 KB,
patch
|
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
10.32 KB,
patch
|
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision eea6188b9b05 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --enable-debug, run with --thread-count=2): for ( shiftpow = 0; shiftpow < 33; shiftpow++ ) for ( addpow = 0; addpow < 33; addpow++ ) Or( 0, 0 ) function ToInt32() { if (__lookupSetter__) {} } function ToInt32BitString( n ) { var b = ""; for ( p = 30; p >= 0; p-- ) { 1 * n >= Math.pow(2,p) b += 0; } return b; } function Or(s, a) { ToInt32(a); var bs = ToInt32BitString(s); for ( var bit = 0; bit < bs.length; bit++ ) {} } Backtrace: Program received signal SIGSEGV, Segmentation fault. js::jit::JitFrameIterator::checkInvalidation (this=<optimized out>, ionScriptOut=ionScriptOut@entry=0x7fffffffae30) at js/src/jit/JitFrames.cpp:163 163 MOZ_ASSERT(ionScript->containsReturnAddress(returnAddr)); #0 js::jit::JitFrameIterator::checkInvalidation (this=<optimized out>, ionScriptOut=ionScriptOut@entry=0x7fffffffae30) at js/src/jit/JitFrames.cpp:163 #1 0x0000000000858a32 in js::jit::JitFrameIterator::ionScript (this=0x7fffffffb210) at js/src/jit/JitFrames.cpp:2261 #2 0x000000000085937a in js::jit::SnapshotIterator::SnapshotIterator (this=0x7fffffffaea0, iter=...) at js/src/jit/JitFrames.cpp:1707 #3 0x0000000000859e00 in js::jit::InlineFrameIterator::resetOn (this=0x7fffffffb240, iter=<optimized out>) at js/src/jit/JitFrames.cpp:2342 #4 0x0000000000865ba3 in js::jit::GetPcScript (cx=0x1a29b50, scriptRes=0x7fffffffb920, pcRes=0x0) at js/src/jit/JitFrames.cpp:1581 #5 0x00000000006213b1 in JSContext::currentScript (this=0x1a29b50, ppc=0x0, allowCrossCompartment=JSContext::ALLOW_CROSS_COMPARTMENT) at js/src/jscntxtinlines.h:462 #6 0x0000000000a09828 in JSContext::findVersion (this=0x1a29b50) at js/src/jscntxt.cpp:1124 #7 0x0000000000a099f0 in JS::CompileOptions::CompileOptions (this=0x7fffffffbbc0, cx=0x1a29b50, version=<optimized out>) at js/src/jsapi.cpp:3633 #8 0x0000000000ad5417 in js::CloneScript (cx=0x1a29b50, enclosingScope=0x0, fun=(JSFunction * const) 0x7ffff565f1c0 [object Function "__lookupSetter__"], src=0x7ffff5646128, newKind=<optimized out>) at js/src/jsscript.cpp:3059 #9 0x0000000000686a16 in JSRuntime::cloneSelfHostedFunctionScript (this=<optimized out>, cx=0x1a29b50, name=..., targetFun=(JSFunction * const) 0x7ffff565f1c0 [object Function "__lookupSetter__"]) at js/src/vm/SelfHosting.cpp:1340 #10 0x0000000000ad4018 in JSFunction::createScriptForLazilyInterpretedFunction (cx=0x1a29b50, fun=(JSFunction * const) 0x7ffff565f1c0 [object Function "__lookupSetter__"]) at js/src/jsfun.cpp:1520 #11 0x000000000041c69f in JSFunction::getOrCreateScript (this=<optimized out>, cx=<optimized out>) at js/src/shell/../jsfun.h:291 #12 0x00000000005e6cc2 in JSObject::makeLazyGroup (cx=0x1a29b50, obj=(JSObject * const) 0x7ffff565f1c0 [object Function "__lookupSetter__"]) at js/src/vm/ObjectGroup.cpp:296 #13 0x00000000004791c0 in JSObject::getGroup (this=(JSObject * const) 0x7ffff565f1c0 [object Function "__lookupSetter__"], cx=0x1a29b50) at js/src/jsobjinlines.h:96 #14 0x00000000007115f8 in js::HeapTypeSetKey::instantiate (this=0x1b311e8, cx=0x1a29b50) at js/src/vm/TypeInference.cpp:1136 #15 0x00000000007119bc in (anonymous namespace)::CompilerConstraintInstance<{anonymous}::ConstraintDataFreezeObjectFlags>::generateTypeConstraint(JSContext *, js::RecompileInfo) (this=0x1b311e0, cx=0x1a29b50, recompileInfo=...) at js/src/vm/TypeInference.cpp:1051 #16 0x00000000006f021e in js::FinishCompilation (cx=0x1a29b50, script=0x7ffff565d1f0, constraints=0x1b2e6d0, precompileInfo=0x7fffffffc350) at js/src/vm/TypeInference.cpp:1238 #17 0x00000000007ebdf4 in js::jit::CodeGenerator::link (this=0x7fffe4001400, cx=0x1a29b50, constraints=0x1b2e6d0) at js/src/jit/CodeGenerator.cpp:7395 #18 0x0000000000879382 in js::jit::LazyLinkTopActivation (cx=0x1a29b50) at js/src/jit/Ion.cpp:474 #19 0x00007ffff558e3f8 in ?? () #20 0x000000000000001f in ?? () #21 0x00007fffffffc680 in ?? () #22 0x0000000000000000 in ?? () rax 0x2 2 rbx 0x7ffff558e3b7 140737309631415 rcx 0xe0ff00000002e800 -2234066890152286208 rdx 0x0 0 rsi 0x7fffffffae30 140737488334384 rdi 0x7ffff5672140 140737310564672 rbp 0x7fffffffae20 140737488334368 rsp 0x7fffffffae10 140737488334352 r8 0x0 0 r9 0x0 0 r10 0x1a72648 27731528 r11 0x1b 27 r12 0x7fffffffae30 140737488334384 r13 0x7fffffffb920 140737488337184 r14 0x0 0 r15 0x7ffff558e3b7 140737309631415 rip 0x857631 <js::jit::JitFrameIterator::checkInvalidation(js::jit::IonScript**) const+97> => 0x857631 <js::jit::JitFrameIterator::checkInvalidation(js::jit::IonScript**) const+97>: mov (%rcx),%rdx 0x857634 <js::jit::JitFrameIterator::checkInvalidation(js::jit::IonScript**) const+100>: mov (%rdx),%rax The testcase reproduces intermittently, you will have to run it a lot of times (100 should suffice to get 2-3 crashes). Marking s-s and sec-critical due to the bad address involved here.
Reporter | ||
Updated•8 years ago
|
Group: core-security
Comment 1•8 years ago
|
||
Nicolas, is this something you can look at? Thanks.
Flags: needinfo?(nicolas.b.pierron)
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #1) > Nicolas, is this something you can look at? Thanks. Yes, I already started to look at it.
Assignee | ||
Comment 3•8 years ago
|
||
Ok, the problem is the following: #18 0x0000000000879382 in js::jit::LazyLinkTopActivation (cx=0x1a29b50) at js/src/jit/Ion.cpp:474 #19 0x00007ffff558e3f8 in ?? () In JIT code, we call the LazyLink function, which is in charge of linking a function. But at this time, we might GC, report errors and many more. All of which might iterate the stack and query what is the status of the top-level frame. When the Jit made the call, it is assumed that the function is jitted, and only the LazyLinkTopActivation will link and bounce to the compiled link. In the mean time, the latest JIT frame makes a reference to a JSScript which is supposed to have an IonScript pointer, which is currently the LazyLinkStub. Thus we fall under the wrong path of checkInvalidation. The approach, I think, would be to mutate the JSFrame into an another frame until the code is properly linked.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Flags: needinfo?(nicolas.b.pierron)
Assignee | ||
Comment 4•8 years ago
|
||
Tested ~11143 times, no longer able to reproduce it with this patch. The problem is that we can iterate / look at the first frame which is one the stack within the LazyLinkStub. While under LazyLinkTopActivation, we have the exit frame, and then the frame that we are trying to link. Unfortunately, the return address of the exit frame targets the code of the LazyLinkStub, and the IonScript might not be linked yet, when we are trying to inspect the stack. To avoid such unexpected invariant failure, this patch mutate the JitFrameLayout to be used as an exit frame for the LazyLinkTopActivation function. Then, when we return from this function we mutate it back to be a normal JitFrameLayout. This way, we do not have to change any of the invariants of the stack iteration. On the other hand, we have to handle this frame properly in case of GCs by marking the arguments of the revamp JitFrameLayout, as well as its callee token. We also have to keep track of the LazyLinkStub, as this was implicit with the enterExitFrame previously.
Attachment #8573390 -
Flags: review?(hv1989)
Reporter | ||
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
Reporter | ||
Comment 5•8 years ago
|
||
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Comment 6•8 years ago
|
||
Comment on attachment 8573390 [details] [diff] [review] LazyLinkStub stops making a call and reuses the parent frame. Review of attachment 8573390 [details] [diff] [review]: ----------------------------------------------------------------- Good find. My initial prototype was doing something similar, until if it was really necessary to have a new frame type. Here I made a wrong assessment and thought it actually wasn't needed. Thanks for fixing this.
Attachment #8573390 -
Flags: review?(hv1989) → review+
Comment 7•8 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #6) > Comment on attachment 8573390 [details] [diff] [review] > LazyLinkStub stops making a call and reuses the parent frame. > > Review of attachment 8573390 [details] [diff] [review]: > ----------------------------------------------------------------- > > Good find. My initial prototype was doing something similar, until if it was > really necessary to have a new frame type. > Here I made a wrong assessment and thought it actually wasn't needed. Thanks > for fixing this. s/until/until asked
Reporter | ||
Updated•8 years ago
|
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Assignee | ||
Comment 8•8 years ago
|
||
This issue is caused by Bug 1047346.
Assignee | ||
Comment 9•8 years ago
|
||
I tried to find similar signatures on crash-stat (~140), I found other issues with checkInvalidation / SnapshotIterator which are not related to this change. We should backport this patch, but I think this code is hard to exploit, as the following stack suggest, that we are mostly doing invalid reads. #4 0x0000000000865ba3 in js::jit::GetPcScript (cx=0x1a29b50, scriptRes=0x7fffffffb920, pcRes=0x0) at js/src/jit/JitFrames.cpp:1581 #5 0x00000000006213b1 in JSContext::currentScript (this=0x1a29b50, ppc=0x0, allowCrossCompartment=JSContext::ALLOW_CROSS_COMPARTMENT) at js/src/jscntxtinlines.h:462 #6 0x0000000000a09828 in JSContext::findVersion (this=0x1a29b50) at js/src/jscntxt.cpp:1124 before returning one incorrect value.
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8573390 [details] [diff] [review] LazyLinkStub stops making a call and reuses the parent frame. > [Security approval request comment] > How easily could an exploit be constructed based on the patch? Hard (see comment 9) > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? While the comments are still related to the stack, it does not specify under which context these can be used. > Which older supported branches are affected by this flaw? All after Gecko 35. > If not all supported branches, which bug introduced the flaw? Bug 1047346 > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Not yet, the patch should be almost the same. > How likely is this patch to cause regressions; how much testing does it need? Tested locally ~11143 times (used to be intermittent ~1/100).
Attachment #8573390 -
Flags: sec-approval?
Updated•8 years ago
|
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → affected
status-firefox-esr31:
--- → unaffected
Updated•8 years ago
|
Comment 11•8 years ago
|
||
Comment on attachment 8573390 [details] [diff] [review] LazyLinkStub stops making a call and reuses the parent frame. sec-approval+ for trunk. We'll want patches for Aurora and Beta as well.
Attachment #8573390 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b83369a62aca
Comment 13•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b83369a62aca
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Updated•8 years ago
|
Flags: in-testsuite?
Comment 14•8 years ago
|
||
(In reply to Al Billings [:abillings] from comment #11) > sec-approval+ for trunk. We'll want patches for Aurora and Beta as well. Moreover, I would highly prefer to take this fix in Beta 6, which gtb on Monday. If branch patches are required, can you get them ready by midday on Monday?
Flags: needinfo?(nicolas.b.pierron)
Reporter | ||
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 15•8 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Comment 17•8 years ago
|
||
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #14) > (In reply to Al Billings [:abillings] from comment #11) > > sec-approval+ for trunk. We'll want patches for Aurora and Beta as well. > > Moreover, I would highly prefer to take this fix in Beta 6, which gtb on > Monday. If branch patches are required, can you get them ready by midday on > Monday? attachment 8578007 [details] [diff] [review] should cover beta.
Flags: needinfo?(nicolas.b.pierron)
Comment 19•8 years ago
|
||
Comment on attachment 8578007 [details] [diff] [review] (beta) LazyLinkStub stops making a call and reuses the parent frame. Approved for Beta and Aurora. Sec questionnaire is in comment 10.
Attachment #8578007 -
Flags: approval-mozilla-beta+
Updated•8 years ago
|
Attachment #8578008 -
Flags: approval-mozilla-aurora+
Comment 20•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/d1add43a60ec https://hg.mozilla.org/releases/mozilla-beta/rev/44cc57c29710
Reporter | ||
Updated•8 years ago
|
Reporter | ||
Comment 22•8 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx38
Updated•8 years ago
|
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main37+]
Updated•8 years ago
|
Group: core-security → core-security-release
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•