Carsten Book [:Tomcat] Reporter
	Description • 9 years ago

found by bughunter and reproduced on a win7 debug system on http://flight.qunar.com/site/interroundtrip_compare.htm?fromCity=%C3%83%C2%A5%C3%82%C2%AE%C3%82%C2%89%C3%83%C2%A6%C3%82%C2%9B%C3%82%C2%BC&toCity=%C3%83%C2%A9%C3%82%C2%A6%C3%82%C2%99%C3%83%C2%A6%C3%82%C2%B8%C3%82%C2%AF&fromDate=2015-06-17&toDate=2015-06-23&fromCode=AMM&toCode=HKG&from=from_rtrip_qiri&lowestPrice=null&isInter=true&favoriteKey=&showTotalPr=1

Steps to reproduce:

-> Load http://flight.qunar.com/site/interroundtrip_compare.htm?fromCity=%C3%83%C2%A5%C3%82%C2%AE%C3%82%C2%89%C3%83%C2%A6%C3%82%C2%9B%C3%82%C2%BC&toCity=%C3%83%C2%A9%C3%82%C2%A6%C3%82%C2%99%C3%83%C2%A6%C3%82%C2%B8%C3%82%C2%AF&fromDate=2015-06-17&toDate=2015-06-23&fromCode=AMM&toCode=HKG&from=from_rtrip_qiri&lowestPrice=null&isInter=true&favoriteKey=&showTotalPr=1

->> Assertion failure: MIR instruction returned object with unexpected type, at c:/mozilla/builds/nightly/mozilla/js/src/jit/MacroAssembler.cpp:1747

exploitable rated this as medium exploitable. Related to bug 1174163 ?

	Nicolas B. Pierron [:nbp]
	Comment 1 • 9 years ago

I was able to reproduce this Crash on a custom build including Bug 1174322, Bug 1174547, and Bug 1175233 patches.

This might be a duplicate of Bug 1175397.

	Nicolas B. Pierron [:nbp]
	Comment 2 • 9 years ago

I am still able to reproduce this crash with Bug 1175397, Bug 1174163, in addition to the previous patches.

Based on the CRASH_REASON, I guess this might be a duplicate of Bug 1174545.

	Nicolas B. Pierron [:nbp]
	Comment 3 • 9 years ago

Hum … I have a fix for Bug 1174545, but it does not seems to be enough to fix this issue.
I will investigate.

	Carsten Book [:Tomcat] Reporter
	Comment 4 • 9 years ago

bughunter found 2 crashes with the same assertion but different signature on http://flight.qunar.com/site/interroundtrip_compare.htm?fromCity=%C3%83%C2%A5%C3%82%C2%BE%C3%82%C2%90%C3%83%C2%A5%C3%82%C2%B7%C3%82%C2%9E&toCity=%C3%83%C2%A5%C3%82%C2%8F%C3%82%C2%B0%C3%83%C2%A5%C3%82%C2%8C%C3%82%C2%97&fromDate=2015-06-25&toDate=2015-07-02&fromCode=XUZ&toCode=TPE&from=flight_int_search&ex_track=auto_50e69a47&lowestPrice=null&isInter=true&favorit

one signature is: js::array_join std::_Atomic_store_4 and the other js::Activation::asJit js::Activation::isProfiling js::Activation::registerProfiling js::jit::JitActivation::JitActivation EnterIon

	Carsten Book [:Tomcat] Reporter
	Comment 6 • 9 years ago

after talking to nbp this might be 2 different things so filed:

(In reply to Carsten Book [:Tomcat] from comment #4
> 
> one signature is: js::array_join std::_Atomic_store_4 

this as bug 1178767

and the other
> js::Activation::asJit js::Activation::isProfiling
> js::Activation::registerProfiling js::jit::JitActivation::JitActivation
> EnterIon

as Bug 1178764

	Nicolas B. Pierron [:nbp]
	Comment 7 • 9 years ago

I will see if I can reproduce the issues reported in comment 0, but I think these got removed by the removal of a few patches: https://hg.mozilla.org/integration/mozilla-inbound/rev/b772e603c42f

	Carsten Book [:Tomcat] Reporter
	Comment 8 • 9 years ago

Bughunter found another assertion but this time with XUL!EnterBaseline(JSContext*, js::jit::EnterJitData&) [BaselineJIT.cpp : 124 + 0xf0] on http://www.autobusubilietai.lt crashing aurora builds.

nbp: shall i file a new bug for this one ?

	Carsten Book [:Tomcat] Reporter
	Comment 9 • 9 years ago

(In reply to Carsten Book [:Tomcat] from comment #8)

> nbp: shall i file a new bug for this one ?

filed now as bug 1181166

	Andrew McCreight [:mccr8]
	Comment 10 • 9 years ago

I'll just mark this sec-high. Feel free to close it if you've fixed the various underlying issues. Thanks!

	Nicolas B. Pierron [:nbp]
	Comment 11 • 9 years ago

I managed to reproduce another issue using the same website as comment 0, by setting javascript.options.ion.offthread_compilation to false.

This issue is not related Scalar Replacement, as I first thought.

	Nicolas B. Pierron [:nbp]
	Comment 12 • 9 years ago

Ok, I manage to extract a shell test case, and see that this was in fact related to my stack of patches for re-landing Bug 1165348.  I will minimize the test case and fix it, and check if I still have issues with this website without --ion-offthread-compile=off.

I will open this bug as soon as I verified that this is indeed the same bug, as the signature with the off-thread compilation disabled looks more like Bug 1174545.

	Nicolas B. Pierron [:nbp]
	Comment 13 • 9 years ago

After minimizing (an obfuscated website into a minimal) the test case, I managed to isolate the issue to something which is unrelated with my stack of patches.

The following test case SIGTRAP on inbound (x64, debug), when run with --ion-offthread-compile=off:


setJitCompilerOption("ion.warmup.trigger", 21);
function revIter(str) {
  str = str.split("").reverse().join("");
  var len = str.length;
  for (var i = 0; i < len; ++i) {
  }
};
revIter("osTafaGU");
revIter("YDtaeK");
revIter("IgQXteVTQXmiVe$");
revIter("");

	Nicolas B. Pierron [:nbp]
	Comment 14 • 9 years ago

I ran a bisect on the minimal test case:

exec: /home/nicolas/mozilla/short-dev/js/src/_build/js-bd8b38a-dbg-x64-gcc48 --ion-offthread-compile=off ../lithium/bug1175538.js
4cee2c0ad4f54c0d6ce73043cfd721a0554631fe is the first bad commit
commit 4cee2c0ad4f54c0d6ce73043cfd721a0554631fe
Author: Brian Hackett <bhackett1024@gmail.com>
Date:   Sat Jun 13 08:10:55 2015 -0700

    Bug 1162986 - Allow objects to be turned into singletons dynamically, r=jandem.


Brian, does that sounds plausible?

	Brian Hackett [Laid off!] Assignee
	Comment 15 • 9 years ago

Thanks for isolating the issue.  This is a regression from bug 1170372, actually, there's a path where the result of a string.split VM call had the wrong group.

	Jan de Mooij [:jandem]
	Comment 16 • 9 years ago

Comment on attachment 8642566 [details] [diff] [review]
patch

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

In str_split, after /* Step 16. */, can we MOZ_ASSERT(aobj->group() == group)? r=me with that.

	Brian Hackett [Laid off!] Assignee
	Comment 17 • 9 years ago

Revised patch per review.

	Brian Hackett [Laid off!] Assignee
	Comment 18 • 9 years ago

Comment on attachment 8643052 [details] [diff] [review]
revised

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

not easily.

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?

aurora.

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

bug 1170372

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?

not likely.

Approval Request Comment
[Feature/regressing bug #]: bug 1170372
[User impact if declined]: potential exploit
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low

	Al Billings [:abillings - ex-MoCo]
	Comment 19 • 9 years ago

Comment on attachment 8643052 [details] [diff] [review]
revised

Approvals given.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce747f01b72
https://hg.mozilla.org/releases/mozilla-aurora/rev/f57b52fa0a58

For future reference, it's very helpful when you attach patches with proper commit information when using checkin-needed.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 21 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/7ce747f01b72

	Ada [:adalucinet]
	Comment 23 • 9 years ago

Reproduced with 41.0a1 asan build under Ubuntu 13.10 64-bit with STR from comment 0 ⇒ ‘Assertion failure: MIR instruction returned object with unexpected type, at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/js/src/jit/MacroAssembler.cpp:1747” displayed in Terminal and the tab crashed.

The above assertion and the tab crash are no longer displayed with 41 RC (Build ID: 20150914162311) nor with latest 42.0a2 debug builds, under Ubuntu 13.10 64-bit. Although, I hit 2 potential issues:
1. While loading the URL from comment 8 I get the following assertion:
“[6105] ###!!! ASSERTION: Adding a child where we already have a child? This may misbehave: 'Error', file /builds/slave/m-rel-l64-d-000000000000000000/build/docshell/shistory/nsSHEntry.cpp, line 665”
I suppose it could be the same as bug 1181166? 

2. “WARNING: aTargetFrame should be related with aTargetContent: '!aTargetFrame || !aTargetFrame->GetContent() || aTargetFrame->GetContent() == aTargetContent || aTargetFrame->GetContent()->GetFlattenedTreeParent() == aTargetContent', file /builds/slave/m-aurora-l64-d-000000000000000/build/src/dom/events/EventStateManager.cpp, line 492” displayed in Terminal while loading the attached URL (from comment 0). Maybe related with bug 1168688?

Brian, what's your take on the above? Thanks in advance!

	Brian Hackett [Laid off!] Assignee
	Comment 24 • 9 years ago

I have no idea what those are, but they aren't related to this patch.

	Ada [:adalucinet]
	Comment 25 • 9 years ago

(In reply to Brian Hackett (:bhackett) from comment #24)
> I have no idea what those are, but they aren't related to this patch.

Thanks! Marking accordingly since the mentioned issues are not related and already tracked separately.