943852 - Crash on Heap with SIGTRAP and TypedObject

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 77b5c6edfe96 (run with --fuzzing-safe --ion-eager):


var T = TypedObject;
var Point = new T.ArrayType(T.float32, 3);
var Line = new T.StructType({from: Point, to: Point});
var Lines = new T.ArrayType(Line, 3);
var lines = new Lines([
  {from: [1, 2, 3], to: [4, 5, 6]},
  {from: [7, 8, 9], to: [10, 11, 12]},
  {from: [13, 14, 15], to: [16, 17, 18]}
]);
function allPoints(lines, func) {
  var handle = Point.handle();
  for (var i = 0; i < lines.length; i++) {
    T.Handle.move(handle, lines, i, "from");
    func(handle[1]);
  }
}
allPoints(lines, function(p) {});

Comment 1

[Christian Holler (:decoder)]

Marked s-s because this crashes with a SIGTRAP (and no range analysis debugging options are in use). This could indicate that we're jumping somewhere in memory where we shouldn't be jumping.

Comment 2

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 8648aa476eef).
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/669ccf5fb31e
user:        Felix S. Klock II
date:        Tue Nov 05 09:53:00 2013 +0100
summary:     Bug 933269: jit support for getElem in TypedObjects (r=nmatsakis).

This iteration took 1.563 seconds to run.

Comment 3

[Christian Holler (:decoder)]

Felix, did bug 933269 really fix this security bug, or did it just affect how the test here reproduces?

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

I don't think bug 933269 really fixed the security bug here.

As Christian hypothesized, it probably has just perturbed the test necessary to expose the bug.

But then again, its not immediately clear from the test what the root cause is/was.  (e.g. my first instinct is to wonder if handles are the root cause, not array access...)

Comment 5

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

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/39bfcadd6492
user:        Hannes Verschore
date:        Tue Nov 26 23:21:18 2013 +0100
summary:     Bug 942105 - IonMonkey: Remove the inlineUseCountRatio option, r=jandem

Bug 942105 probably fixed this. Hannes, does this seem plausible?

Comment 6

[Christian Holler (:decoder)]

Needinfo from Hannes per comment 5 :) Hannes, is that the right fix?

Comment 7

[Hannes Verschore [:h4writer]]

(In reply to Christian Holler (:decoder) from comment #6)
> Needinfo from Hannes per comment 5 :) Hannes, is that the right fix?

No, it only changes when we compile functions. So we should still see this.

Comment 8

[Hannes Verschore [:h4writer]]

Looked a bit closer. Bug 947188 would have given a better clue on what is failing: "Argument check fail."

Ionmonkey is assuming some types for the arguments. We have some code that allow us to remove the argument type checks if we can guarantee the types are correct. We do that here. But seems like they aren't correct!

This is very bad. I've added that code and I've debugged similar issues. So it might be better for me to take over?

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: bug943852-pTv1-tests.patch

Regression tests for the bug.

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch F v1: the fix for the bug

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8348715 [details] [diff] [review]
bug943852-pTv1-tests.patch

These cover both the getElemTryScalar and the getPropTryScalar cases.  (Both of which are fixed in patch F v1, attachment 8348716 [details] [diff] [review] )

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8348715 [details] [diff] [review]
bug943852-pTv1-tests.patch

switching review to h4writer in hopes of speedy feedback loop.

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8348716 [details] [diff] [review]
patch F v1: the fix for the bug

Switching r? to h4writer in hopes of speedy feedback loop.

Comment 14

[Hannes Verschore [:h4writer]]

Comment on attachment 8348715 [details] [diff] [review]
bug943852-pTv1-tests.patch

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

Testcases are auto r+ ;)

Comment 15

[Hannes Verschore [:h4writer]]

Comment on attachment 8348716 [details] [diff] [review]
patch F v1: the fix for the bug

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

Please adjust which versions are exploitable. And ask for sec-approval.

::: js/src/jit/IonBuilder.cpp
@@ +6666,5 @@
>      // never executed. The known pushed type is only used to distinguish
>      // uint32 reads that may produce either doubles or integers.
>      types::TemporaryTypeSet *resultTypes = bytecodeTypes(pc);
>      bool allowDouble = resultTypes->hasType(types::Type::DoubleType());
> +    // Note: knownType is not necessarily an element of resultTypes.

I reasoned this only happens in one case: if the function returns a double that fits an integer. We will record an integer instead of the double. So that's what happens here.

(Feel free to add this information or adjust the line having that information.)

Note: if this is exploitable on FF28, please land without this note included! Since it will give extra information on how to exploit.

Comment 16

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore [:h4writer] from comment #14)
> Comment on attachment 8348715 [details] [diff] [review]
> bug943852-pTv1-tests.patch
> 
> Review of attachment 8348715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Testcases are auto r+ ;)

And only land if every exploitable version is fixed. And it doesn't hurt to even wait some time. I often wait for a whole release cycle before landing the exploitable tests. But that's up to you. In this case it might not make sense. No released version is exploitable.

Comment 17

[Niko Matsakis [:nmatsakis]]

Typed objects are only accessible in nightly builds, so it should be all right to release without security approval and so on, at least I understand the policy.

Comment 18

[Niko Matsakis [:nmatsakis]]

Correction: ...at least AS I understand the policy.

Comment 19

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8348716 [details] [diff] [review]
patch F v1: the fix for the bug

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

-> It cannot; this code is not reachable in FF 28.

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

-> I do not think it is necessary, since I believe the only code that has this mistake is the Typed Object code, which is unreachable in FF 28.  (However, I can remove the comments if it is deemed necessary for risk reduction.)

Which older supported branches are affected by this flaw?

-> None.

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

-> Bug 933269; but its code (i.e. this code) is not reachable from FF 28 (Aurora).

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

-> It is not necessary.

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

-> Very unlikely.

Comment 20

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

Comment on attachment 8348716 [details] [diff] [review]
patch F v1: the fix for the bug

sec-approval+ though if the code is effectively ifdef'd out on branches, you don't really need my approval to land. :-)

Comment 21

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8348716 [details] [diff] [review]
patch F v1: the fix for the bug

Landed patch F on inbound:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/0c5ba564c2b7

I will land the tests separately.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c5ba564c2b7

Comment 23

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.