Closed Bug 1011745 Opened 5 years ago Closed 5 years ago

Crash [@ js::jit::BacktrackingAllocator::tryGroupRegisters] or Assertion failure: entryDef->block() == this, at jit/MIRGraph.cpp or Assertion failure: oldDef == entry->getSlot(slot), at jit/IonBuilder.cpp

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox31 --- unaffected
firefox32 + verified
firefox33 + verified
firefox-esr24 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: gkw, Assigned: nbp)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(4 files)

Attached file stack
Function("for (p in u) { [0 for each (z in[]) if(0)] }")()

asserts js debug shell on m-c changeset 616dc757d98a with --ion-eager --ion-parallel-compile=off at Assertion failure: entryDef->block() == this, at jit/MIRGraph.cpp

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140515224929" and the hash "5385817bdae6".
The "bad" changeset has the timestamp "20140515225832" and the hash "5e18fd30243f".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5385817bdae6&tochange=5e18fd30243f

Nicolas, is bug 1007027 a likely regressor?

Setting this sec-critical (feel free to change if necessary) and s-s because it is an assert in MIR.
Flags: needinfo?(nicolas.b.pierron)
Attached file stack
Function("\
    for (var y = 0; y < 1; ++y) {\
        [z for each(x in []) if (0)];\
    }\
")()


Asserts 64-bit debug deterministic threadsafe js shells on m-c changeset rev 41a54c8add09 with --ion-eager --ion-parallel-compile=off at Assertion failure: oldDef == entry->getSlot(slot), at jit/IonBuilder.cpp
Summary: Assertion failure: entryDef->block() == this, at jit/MIRGraph.cpp → Assertion failure: entryDef->block() == this, at jit/MIRGraph.cpp or Assertion failure: oldDef == entry->getSlot(slot), at jit/IonBuilder.cpp
Jandem, do you have any suggestions for who could look into this?  Thanks.
Flags: needinfo?(jdemooij)
(In reply to Andrew McCreight [:mccr8] from comment #2)
> Jandem, do you have any suggestions for who could look into this?  Thanks.

Nicolas, is autoBisect right and is this a regression from bug 1007027?
Flags: needinfo?(jdemooij)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Created attachment 8424197 [details]
> stack

This is amazing !!!
You managed to fail on the assertion inside inheritPhisFromBackedge [1], knowing that this call is guarded by the same assertion in setBackedge [2].

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIRGraph.cpp?from=inheritPhisFromBackedge#1180
[2] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIRGraph.cpp#968

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #1)
> Created attachment 8424599 [details]
> stack

This is indeed an assertion added as part of bug 1007027.  I will check on Monday if I did not miss a case which make this assertion invalid or if there is a silent bug hiding behind this failure.
(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> (In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> > Created attachment 8424197 [details]
> > stack
> 
> This is amazing !!!
> You managed to fail on the assertion inside inheritPhisFromBackedge [1],
> knowing that this call is guarded by the same assertion in setBackedge [2].

oh, my bad, I got miss led by the quoted sources and the wrong pc.
This assertion was there before I just moved it to this new function.  It used to be in the setBackedge[1] function before.

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/5e18fd30243f#l7.106
Flags: needinfo?(nicolas.b.pierron)
nbp: are still investigating this sec-crit bug?
Flags: needinfo?(nicolas.b.pierron)
Keywords: reproducible
(In reply to Chris Peterson (:cpeterson) from comment #6)
> nbp: are still investigating this sec-crit bug?

Not for the moment.
I will try to look at it this week.
Assignee: nobody → nicolas.b.pierron
Attached file stack for opt crash
(y ? undefined : (function() {}))
([0 for (x in 0) if (0)])

crashes js opt shell (32-bit threadsafe deterministic) [@ js::jit::BacktrackingAllocator::tryGroupRegisters] and asserts at Assertion failure: entryDef->block() == this, at jit/MIRGraph.cpp

Configuration parameters:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>
Crash Signature: [@ js::jit::BacktrackingAllocator::tryGroupRegisters]
Summary: Assertion failure: entryDef->block() == this, at jit/MIRGraph.cpp or Assertion failure: oldDef == entry->getSlot(slot), at jit/IonBuilder.cpp → Crash [@ js::jit::BacktrackingAllocator::tryGroupRegisters] or Assertion failure: entryDef->block() == this, at jit/MIRGraph.cpp or Assertion failure: oldDef == entry->getSlot(slot), at jit/IonBuilder.cpp
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #8)
> (y ? undefined : (function() {}))
> ([0 for (x in 0) if (0)])
> 
> crashes js opt shell (32-bit threadsafe deterministic) [@
> js::jit::BacktrackingAllocator::tryGroupRegisters] and asserts at Assertion
> failure: entryDef->block() == this, at jit/MIRGraph.cpp

Run this with --ion-parallel-compile=off --ion-regalloc=backtracking --ion-eager. Also, this was tested on m-c rev 4876594bacaa.
This patch change the code which expect that Phi are only introduced by the
loop header block.  With this patch, we only check operands of the entry resume
points of the loop header which are inside the loop header.

This condition should only match loop headers' phi, as entry resume points are either
capturing definitions from the previous basic blocks or Phi from the current
block.  Note that this is asserted by the toPhi() function which is used after.
Attachment #8440031 - Flags: review?(hv1989)
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8440031 [details] [diff] [review]
Correctly distinguish loop header phis.

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

::: js/src/jit/MIRGraph.cpp
@@ +1140,5 @@
>      size_t stackDepth = headerRp->numOperands();
>      for (size_t slot = 0; slot < stackDepth; slot++) {
>          MDefinition *exitDef = getSlot(slot);
>          MDefinition *loopDef = headerRp->getOperand(slot);
> +        if (loopDef->block() != header) {

Hmm. So after careful looking, I understand this also makes sure only Phi's go through. Though it is hard to see immediately. Can you add a comment here and below:

// Only iterate phi's that are created in this block. We don't test for being a Phi,
// since we get that for free. All instructions captured in the entryResumePoint of
// this block can only be phi's.
Attachment #8440031 - Flags: review?(hv1989) → review+
Comment on attachment 8440031 [details] [diff] [review]
Correctly distinguish loop header phis.

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

I don't know.  The patch it-self does not mention that this is related to the generator syntax, still the history is clearly linking the introduction of this branch with the presence of generators.

In tests cases of comment 0 and comment 1, we are manipulating Phi nodes which have only one argument (which is something newer than Bug 1007027).  So now it would be benign (but probably not on aurora), as we only replace a Phi with one operand by its operand.

Comment 8, highlight that we have a crash with the backtracking allocator, which is currently only used for asm.js.

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

Gecko 32.

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

Bug 1007027

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

This should be the same patch.

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

This patch is unlikely to cause any new regression, otherwise it might bring the spotlight of fuzzers on existing one.
Attachment #8440031 - Flags: sec-approval?
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision f19ca5123d6a).
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update]
Comment on attachment 8440031 [details] [diff] [review]
Correctly distinguish loop header phis.

sec-approval+ for trunk. Let's get an Aurora patch for this submitted and nominated as well.
Attachment #8440031 - Flags: sec-approval? → sec-approval+
This is ready to land, at least as far as having sec-approval+.
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8440031 [details] [diff] [review]
Correctly distinguish loop header phis.

This patch applies cleanly on top of aurora without merge conflicts.
I will land it on inbound.
Flags: needinfo?(nicolas.b.pierron)
https://hg.mozilla.org/mozilla-central/rev/2bc6f38f9cff
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
(In reply to Nicolas B. Pierron [:nbp] from comment #12)
> [Security approval request comment]
> > How easily could an exploit be constructed based on the patch?
> 
> I don't know.  The patch it-self does not mention that this is related to
> the generator syntax, still the history is clearly linking the introduction
> of this branch with the presence of generators.
> 
> In tests cases of comment 0 and comment 1, we are manipulating Phi nodes
> which have only one argument (which is something newer than Bug 1007027). 
> So now it would be benign (but probably not on aurora), as we only replace a
> Phi with one operand by its operand.

The debug assertion asserts the same way on Aurora and the Phi of the generators are the same, and opt builds are working fine, except for comment 8.

> Comment 8, highlight that we have a crash with the backtracking allocator,
> which is currently only used for asm.js.

I can reproduce the crash with an opt build on Aurora.
But this configuration is not used by default with generators, and thus this path should not be taken.
Comment on attachment 8440031 [details] [diff] [review]
Correctly distinguish loop header phis.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1007027
User impact if declined: None, but fuzzers might hit this bug instead of real one.
Testing completed (on m-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None.

gkw, decoder: Do you think we should backport this patch, for fuzzing reasons?
Attachment #8440031 - Flags: approval-mozilla-aurora?
Flags: needinfo?(gary)
Flags: needinfo?(choller)
(In reply to Nicolas B. Pierron [:nbp] from comment #21)
> gkw, decoder: Do you think we should backport this patch, for fuzzing
> reasons?

I would think so, as you say you can reproduce the crash. Moreover, this is just for aurora backporting, not beta, so the bar is lower for approval requests.
Flags: needinfo?(gary)
Attachment #8440031 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(choller)
JSBugMon: This bug has been automatically verified fixed on Fx32
Group: javascript-core-security
Group: core-security
You need to log in before you can comment on or make changes to this bug.