Assertion failure: resumePointsEmpty(), at js/src/jit/MIRGraph.cpp:1092

VERIFIED FIXED in Firefox 55

Status

()

defect
--
critical
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: decoder, Assigned: nbp)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla56
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 wontfix, firefox55+ fixed, firefox56+ fixed, firefox57 verified)

Details

(Whiteboard: [jsbugmon:update][adv-main55+][post-critsmash-triage])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
The following testcase crashes on mozilla-central revision e49151136658 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

loadFile(`
function f6(x,x,x,x) {
  var ret = [];
  ret.push(arguments[i]);
}
actual = f6(1,2,3,4);
`);
loadFile(`
do setJitCompilerOption('ion.forceinlineCaches', 1);
while ( f2.arguments );
`);
loadFile(`
actual = f6(1,2,3,4);
`);
function loadFile(lfVarx) {
    try {
        evaluate(lfVarx);
    } catch (lfVare) {    }
}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x000000000077507f in js::jit::MBasicBlock::discardAllResumePoints (this=this@entry=0x7ffff69c6788, discardEntry=discardEntry@entry=true) at js/src/jit/MIRGraph.cpp:1092
#0  0x000000000077507f in js::jit::MBasicBlock::discardAllResumePoints (this=this@entry=0x7ffff69c6788, discardEntry=discardEntry@entry=true) at js/src/jit/MIRGraph.cpp:1092
#1  0x00000000007a1f4f in js::jit::MBasicBlock::clear (this=0x7ffff69c6788) at js/src/jit/MIRGraph.cpp:1106
#2  js::jit::MIRGraph::removeBlock (this=this@entry=0x7ffff69c5050, block=block@entry=0x7ffff69c6788) at js/src/jit/MIRGraph.cpp:263
#3  0x00000000007b5086 in js::jit::MIRGraph::removeSuccessorBlocks (this=0x7ffff69c5050, start=0x7ffff69c56c0) at js/src/jit/MIRGraph.cpp:218
#4  0x00000000007b525f in js::jit::MBasicBlock::BackupPoint::restore (this=this@entry=0x7fffffffac40) at js/src/jit/MIRGraph.cpp:1766
#5  0x00000000006f588b in js::jit::IonBuilder::inlineScriptedCall (this=this@entry=0x7ffff69c51c0, callInfo=..., target=<optimized out>) at js/src/jit/IonBuilder.cpp:3758
#6  0x00000000006f5e27 in js::jit::IonBuilder::inlineSingleCall (this=this@entry=0x7ffff69c51c0, callInfo=..., targetArg=targetArg@entry=0x7ffff46ad940) at js/src/jit/IonBuilder.cpp:4242
#7  0x00000000006f76bb in js::jit::IonBuilder::inlineCallsite (this=this@entry=0x7ffff69c51c0, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:4296
#8  0x00000000006f7b5e in js::jit::IonBuilder::jsop_call (this=this@entry=0x7ffff69c51c0, argc=4, constructing=<optimized out>, ignoresReturnValue=<optimized out>) at js/src/jit/IonBuilder.cpp:5298
#9  0x00000000006fd2c6 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7ffff69c51c0, op=op@entry=JSOP_CALL) at js/src/jit/IonBuilder.cpp:2035
#10 0x00000000006fe9c2 in js::jit::IonBuilder::visitBlock (this=this@entry=0x7ffff69c51c0, cfgblock=cfgblock@entry=0x7ffff4359760, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1539
#11 0x00000000006f3d26 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7ffff69c51c0) at js/src/jit/IonBuilder.cpp:1456
#12 0x00000000006f49ce in js::jit::IonBuilder::build (this=this@entry=0x7ffff69c51c0) at js/src/jit/IonBuilder.cpp:846
#13 0x000000000043340d in js::jit::IonCompile (cx=cx@entry=0x7ffff6924000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=osrPc@entry=0x0, recompile=<optimized out>, optimizationLevel=<optimized out>) at js/src/jit/Ion.cpp:2176
#14 0x000000000070795b in js::jit::Compile (cx=cx@entry=0x7ffff6924000, script=script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2440
#15 0x0000000000707af8 in js::jit::CanEnter (cx=cx@entry=0x7ffff6924000, state=...) at js/src/jit/Ion.cpp:2537
#16 0x0000000000536d4a in js::RunScript (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:386
#17 0x00000000005398b9 in js::ExecuteKernel (cx=cx@entry=0x7ffff6924000, script=..., script@entry=..., envChainArg=..., newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=result@entry=0x7fffffffc1a8) at js/src/vm/Interpreter.cpp:699
#18 0x0000000000539e10 in js::Execute (cx=cx@entry=0x7ffff6924000, script=script@entry=..., envChainArg=..., rval=rval@entry=0x7fffffffc1a8) at js/src/vm/Interpreter.cpp:732
#19 0x00000000008e01ca in ExecuteScript (cx=cx@entry=0x7ffff6924000, scope=scope@entry=..., script=script@entry=..., rval=rval@entry=0x7fffffffc1a8) at js/src/jsapi.cpp:4616
#20 0x00000000008eff88 in JS_ExecuteScript (cx=cx@entry=0x7ffff6924000, scriptArg=scriptArg@entry=..., rval=...) at js/src/jsapi.cpp:4642
#21 0x000000000045dded in Evaluate (cx=0x7ffff6924000, argc=<optimized out>, vp=<optimized out>) at js/src/shell/js.cpp:1656
#22 0x00003f9d2c232625 in ?? ()
#23 0x00007ffff697e140 in ?? ()
#24 0x00007fffffffc180 in ?? ()
#25 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff69c6788	140737330833288
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffaab0	140737488333488
rsp	0x7fffffffaaa0	140737488333472
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x1	1
r13	0x7ffff69c5050	140737330827344
r14	0x7ffff69c6788	140737330833288
r15	0x7fffffffab40	140737488333632
rip	0x77507f <js::jit::MBasicBlock::discardAllResumePoints(bool)+127>
=> 0x77507f <js::jit::MBasicBlock::discardAllResumePoints(bool)+127>:	movl   $0x0,0x0
   0x77508a <js::jit::MBasicBlock::discardAllResumePoints(bool)+138>:	ud2    



Marking s-s until this has been investigated by JS developers. The assertion is in JIT code and I don't know what the implications are if something is wrong there with the inline cache.
(Assignee)

Updated

2 years ago
Blocks: 1373323
Flags: needinfo?(nicolas.b.pierron)

Updated

2 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

2 years ago
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/603e5ded7f5d
user:        Nicolas B. Pierron
date:        Mon Jun 19 16:29:51 2017 +0000
summary:     Bug 1373323 - IonMonkey: Enable backtracking on inlining failures. r=jandem

This iteration took 270.131 seconds to run.
Hey Nicolas, did you get a chance to look at this yet?
Flags: needinfo?(nicolas.b.pierron)
> Hey Nicolas, did you get a chance to look at this yet?
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 4

2 years ago
The problem is that the GetPropertyCache allocate a prior-resume-point, and we fail before the following call which is supposed to consume it.  We should clean the maybeFallbackFunctionGetter_ field in case of failure.
(Assignee)

Comment 5

2 years ago
This patch fixes the issue by adding MakeScopeExit clean-up code in
IonBuilder::buildInline.

I also added it to IonBuilder::build, such that we do not keep are resume
points alive which is not attached to any basic block. (even if I would not
expect that to happen, knowing that the AssertGraph* functions should have
caught it.)

Most of the dist from this patch comes from moving the WrapMGetPropertyCache
higher in the same file, while keeping one of its function where it used to be.
Attachment #8888439 - Flags: review?(jdemooij)
(Assignee)

Comment 6

2 years ago
Comment on attachment 8888439 [details] [diff] [review]
Clear the GetPropertyCache prior resume-point at the end of IonBuilder build functions.

Review of attachment 8888439 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/IonBuilder.cpp
@@ +881,5 @@
>  #endif
>  
>      insertRecompileCheck();
>  
> +    auto clearLastPriorResumePoint = mozilla::MakeScopeExit([&] {

self-nit: add #include "mozilla/ScopeExit.h" header.
Just to make sure I understand correctly: does this mean we can remove the replaceMaybeFallbackFunctionGetter(nullptr); calls in build() and buildInline()?

What if the ScopeExit just calls builder->replaceMaybeFallbackFunctionGetter(nullptr), can we revert most of the other changes in the patch?
Assignee: nobody → nicolas.b.pierron
(Assignee)

Comment 8

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #7)
> Just to make sure I understand correctly: does this mean we can remove the
> replaceMaybeFallbackFunctionGetter(nullptr); calls in build() and
> buildInline()?

Oh, I did not even noticed it.  This seems to be a bug introduced by the addition of the MOZ_TRY :/

> What if the ScopeExit just calls
> builder->replaceMaybeFallbackFunctionGetter(nullptr), can we revert most of
> the other changes in the patch?

Yes, I will do that a submit another patch.
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Updated

2 years ago
Blocks: 1286505
(Assignee)

Comment 9

2 years ago
Delta:
 - Follow better suggestion.
Attachment #8889439 - Flags: review?(jdemooij)
(Assignee)

Updated

2 years ago
Attachment #8888439 - Attachment is obsolete: true
Attachment #8888439 - Flags: review?(jdemooij)
Comment on attachment 8889439 [details] [diff] [review]
Clear the GetPropertyCache prior resume-point at the end of IonBuilder build functions.

Review of attachment 8889439 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #8889439 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 11

2 years ago
Comment on attachment 8889439 [details] [diff] [review]
Clear the GetPropertyCache prior resume-point at the end of IonBuilder build functions.

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

This is not easy.  I once tried on a similar issue and failed to do so.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

This highlights that we did not clean-up as expected, but it does not give any hint on what pointers one might make us of.

> Which older supported branches are affected by this flaw?

Since Firefox 53 (Bug 1286505).

> If not all supported branches, which bug introduced the flaw?

Bug 1286505 added the MOZ_TRY syntax, which skip the existing code for removing the reference to the unused resume point.

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

The same patch should apply cleanly, or it should be trivial to adapt.

> How likely is this patch to cause regressions; how much testing does it need?

Unexpected.  No testing should be required.
Attachment #8889439 - Flags: sec-approval?
Attachment #8889439 - Flags: approval-mozilla-beta?
Al pinged me about this one. We still have one beta before RC1 and this is a sec-high so it meets the late beta uplift bar.
Comment on attachment 8889439 [details] [diff] [review]
Clear the GetPropertyCache prior resume-point at the end of IonBuilder build functions.

sec-approval+ for trunk and giving Beta approval after talking to Ritu.
Attachment #8889439 - Flags: sec-approval?
Attachment #8889439 - Flags: sec-approval+
Attachment #8889439 - Flags: approval-mozilla-beta?
Attachment #8889439 - Flags: approval-mozilla-beta+

Comment 16

2 years ago
backout
Backed out for making too-long-array-splice.js perma-timeout on SM asan builds (on inbound and Beta both). Note that bug 1368978 was already tracking the same failures happening intermittently.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2801fbe0885c038df8c87b7501445fdb832694a
https://hg.mozilla.org/releases/mozilla-beta/rev/3d3cf32c21d539abac825ddbf8bddb162053b691

https://treeherder.mozilla.org/logviewer.html#?job_id=117769158&repo=mozilla-inbound
Flags: needinfo?(nicolas.b.pierron)
*sigh* The failures are still happening (and looking further back at inbound, they do in fact go back to before this landed). Re-landing on inbound now, will get Beta later. Sorry for the churn, hopefully someone can look at bug 1368978 Really Soon Now before it leads to even more confusion :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5950d83bde758cb389c14ea211babb48899ae67
Flags: needinfo?(nicolas.b.pierron)
https://hg.mozilla.org/mozilla-central/rev/d5950d83bde7

also set a needinfo to get someone looking into bug 1368978
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
This time with feeling.

https://hg.mozilla.org/releases/mozilla-beta/rev/1df81caed459
Target Milestone: --- → mozilla56
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main55+]
Group: javascript-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [jsbugmon:update][adv-main55+] → [jsbugmon:update][adv-main55+][post-critsmash-triage]

Updated

2 years ago
Status: RESOLVED → VERIFIED

Comment 20

2 years ago
JSBugMon: This bug has been automatically verified fixed.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.