1138391 - Crash [@ js::jit::JitFrameIterator::checkInvalidation] or Crash [@ js::jit::SnapshotIterator::SnapshotIterator] with weird address

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Andrew McCreight [:mccr8]]

Nicolas, is this something you can look at?  Thanks.

Comment 2

[Nicolas B. Pierron [:nbp]]

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

Comment 3

[Nicolas B. Pierron [:nbp]]

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.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: LazyLinkStub stops making a call and reuses the parent frame.

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.

Comment 5

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 6

[Hannes Verschore [:h4writer]]

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.

Comment 7

[Hannes Verschore [:h4writer]]

(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

Comment 8

[Nicolas B. Pierron [:nbp]]

This issue is caused by Bug 1047346.

Comment 9

[Nicolas B. Pierron [:nbp]]

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.

Comment 10

[Nicolas B. Pierron [:nbp]]

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

Comment 11

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

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.

Comment 12

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b83369a62aca

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b83369a62aca

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 15

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 16

[Nicolas B. Pierron [:nbp]]

Attachment: (beta) LazyLinkStub stops making a call and reuses the parent frame.

Comment 17

[Nicolas B. Pierron [:nbp]]

Attachment: (aurora) LazyLinkStub stops making a call and reuses the parent frame.

Comment 18

[Nicolas B. Pierron [:nbp]]

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

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d1add43a60ec
https://hg.mozilla.org/releases/mozilla-beta/rev/44cc57c29710

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/44cc57c29710

Comment 22

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx38