879727 - Assertion failure: argnum < argslots_, at ion/Lowering.cpp:2854 or Crash [@ insert]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 8f9ba85eb61c (run with --ion-eager):


try {
function looper(f) {
  for (var i = 0; i < 10; a) {}
}
function f() {
    unescape(false || looper());
}
f();
} catch(exc1) {}
new f();

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Dangerous crash on opt:


Program received signal SIGSEGV, Segmentation fault.
insert (value=<optimized out>, this=<optimized out>) at js/src/ion/BitSet.h:72
72              bits_[wordForValue(value)] |= bitForValue(value);
#0  insert (value=<optimized out>, this=<optimized out>) at js/src/ion/BitSet.h:72
#1  MapSlotsToBitset (set=0x16d3b70, stream=..., nslots=1, slots=0x16d3c00) at js/src/ion/Safepoints.cpp:90
#2  0x000000000077742c in writeValueSlots (safepoint=0x16d1a30, this=0x163a348) at js/src/ion/Safepoints.cpp:125
#3  js::ion::SafepointWriter::encode (this=0x163a348, safepoint=0x16d1a30) at js/src/ion/Safepoints.cpp:286
#4  0x00000000008c1db0 in js::ion::CodeGeneratorShared::encodeSafepoints (this=0x1639c70) at js/src/ion/shared/CodeGenerator-shared.cpp:343
#5  0x00000000006c0f71 in js::ion::CodeGenerator::link (this=0x1639c70) at js/src/ion/CodeGenerator.cpp:5200
#6  0x00000000006c7353 in js::ion::IonCompile (cx=0x16242f0, script=0x7ffff6551280, fp=..., osrPc=0x0, constructing=<optimized out>, executionMode=js::ion::SequentialExecution) at js/src/ion/Ion.cpp:1423
#7  0x00000000006c76b8 in js::ion::Compile (cx=0x16242f0, script=..., fp=..., osrPc=<optimized out>, constructing=<optimized out>, executionMode=js::ion::SequentialExecution) at js/src/ion/Ion.cpp:1580
rax     0x216d3b7c      560806780
rdi     0x80000000      2147483648
=> 0x777062 <MapSlotsToBitset(js::ion::BitSet*, js::ion::CompactBufferWriter&, uint32_t, uint32_t*)+82>:        or     %edi,(%rax)


Marking s-s based on crash. Given that the we're out-of-bounds and calling "writeValueSlots" I assume that we could possibly write something out-of-bounds, so marking sec-critical until shown otherwise.

Comment 3

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

Can we get a bisect to see how far back this goes?

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, failed due to error (try manually).

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/6336284c7f1f
user:        Hannes Verschore
date:        Fri May 10 14:49:58 2013 +0200
summary:     Bug 768288: IonMonkey: Inline small functions with loops, r=djvj r=shu

Comment 6

[Hannes Verschore [:h4writer]]

Just had a look. I made sure that inlining get disabled when we have a infitine loop (in IonBuilder), since we don't have a return value in that case. Now it looks like UCE can also introduce this. This is what happening here. Not sure what the best solution for this is, yet. I'm gonna think about it ... (this is why I'm not clearing needinfo)

Comment 7

[Hannes Verschore [:h4writer]]

Taking.

The problem is with UCE. It can remove a call instruction, but not the corresponding PassArg operands. I have only observed it with UCE and infinite loops, but this is not tied to inlining! It is possible to create a testcase that doesn't require inlining. Therefore removing that dependency and adding UCE.

Fix is to replace the PassArg with the argument when a call gets removed.

Comment 8

[Hannes Verschore [:h4writer]]

Bug 820676 landed in FF20...

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 760911 [details] [diff] [review]
Patch

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

Nice.

::: js/src/ion/UnreachableCodeElimination.cpp
@@ +236,5 @@
>              }
>  
> +            // When we remove a call, we can't leave the corresponding MPassArg in the graph.
> +            // Since lowering will fail. Replace it with the argument for the exceptional
> +            // case where it is kept alive in a ResumePoint.

Nit: add a sentence here: "DCE will remove the unused MPassArg instruction."

@@ +239,5 @@
> +            // Since lowering will fail. Replace it with the argument for the exceptional
> +            // case where it is kept alive in a ResumePoint.
> +            for (MInstructionIterator iter(block->begin()); iter != block->end(); iter++) {
> +                if (iter->isCall()) {
> +                    for (size_t i = 2; i < iter->numOperands(); i++) {

Nit: instead of hardcoding 2 here, use something like:

MCall *call = iter->toCall();

for (size_t i = 0; i < call->numStackArgs(); i++) {
    call->getArg(i);
    ...
}

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Hannes Verschore [:h4writer] from comment #8)
> Bug 820676 landed in FF20...

This probably means our tracking flags should be changed...

Comment 12

[Hannes Verschore [:h4writer]]

After reconsideration, I haven't found a way to exploit this issue before loop inlining was enabled. So setting blocker back on that bug.

Comment 13

[Hannes Verschore [:h4writer]]

Attachment: Patch

Taking r+ from previous patch.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Two important clues (inlining and infinite loop) on how to construct a testcase aren't obvious from the patch. But definitely not impossible. With some dedication possible.

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

Check-in comment: Just describes the code. No extra information in there
Comment in code: Contains a little bit extra information. By far not enough to explain this exploit, though. I can remove it for commit and only push later.
Testcase: Not included, will land later.

Which older supported branches are affected by this flaw?
Faulty code is in tree since FF20, but looks like it could only get exploited since FF23. We were saved by some restrictions. (=> mozilla-central / mozilla-aurora)

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Backport to mozilla-aurora is trivial. Needs only three lines extra. I'll create it after ss+

How likely is this patch to cause regressions; how much testing does it need?
Unlikely. Fairy simple fix

Comment 14

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

Comment on attachment 761361 [details] [diff] [review]
Patch

sec-approval+ for trunk. Please check it in and then nominate an Aurora patch so we can avoid shipping the exploitable issue.

Comment 15

[Hannes Verschore [:h4writer]]

Attachment: Aurora patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f52e55ac5be

Takes r+ from mozilla-central patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): #768288
 
User impact if declined: Possible to crash browser dangerously with special crafted javascript

Testing completed (on m-c, etc.): passes jit-tests, just pushed to m-c

Risk to taking this patch (and alternatives if risky): Low


String or IDL/UUID changes made by this patch: /

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6f52e55ac5be

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ba1a561d6375

Comment 18

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

[u279076]

Marking status-firefox23:verified based on comment 18.

Comment 20

[u279076]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #19)
> Marking status-firefox23:verified based on comment 18.

Sorry, that should have been Fx24, not Fx23.