547299 - TM: Crash in [@ GetPropertyByName ] when mousing over Travel information on Trailways site, or "Assertion failed: s0->isQuad() && s1->isQuad() (../nanojit/LIR.cpp"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marcia Knous [:marcia]]

Seen while running Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a2pre) Gecko/20100219 Minefield/3.7a2pre.

STR:
1. Load the site in the URL 
2. Crash.

Not able to reproduce 100% but it has happened to me at least once and there are several other crash reports for the same site.

Comment 1

[Marcia Knous [:marcia]]

https://crash-stats.mozilla.com/report/index/dbbf2d2b-dac4-43fb-b53a-c56502100219 is my report.

Comment 2

[Ria Klaassen (not reading all bugmail)]

Confirmed on Windows Vista as well:

http://crash-stats.mozilla.com/report/index/9cad21ae-df8f-4fd7-bac0-d34502100219

Comment 3

[Ria Klaassen (not reading all bugmail)]

Also latest Namoroko 3.6 crashes on this site, without report here.

Comment 4

[Ria Klaassen (not reading all bugmail)]

STR: move your mouse over the "Travel Information" on the left side.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Marking security sensitive as a precaution, so I can attach a test case. Will un-mark if we conclude it's not a problem (which is my guess).

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

The bug is that this web page does something like:

  arguments[1] = arguments[0]
  for (var i = 0; i < 2; i++)
     ... bleh[arguments[i]];

This does not change |arguments.length|, so the JIT assumes the out-of-range index will be a JSVAL_VOID. The interpreter, however, puts |arguments[0]| on the stack. So there is a type mismatch between the LIR and the interpreter stack. At this point pretty much anything can go wrong, but on this site, it caused GetPropertyByName to get an address to a boolean instead of an address to a JSString *.

I'm taking the easy fix here and making the trace abort if we try to access arguments out of the |arguments.length| range. It seems like a hard fix would involve expensive guards or some way to flag the Arguments object as having new properties introduced. Since this affects 1.9.2 I'm not too excited about that.

I don't think this is security sensitive, but someone else should chime in. I think the worst that can happen is someone can tag a JSVAL_VOID as a JSString* or JSObject* and let it leak out. On x86 this should look like a NULL deref and on x64 it's more severe (since the top bits could be anything).

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/700e7a9e95ff

leaving test case out until bug is opened.

Comment 8

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

function f(a) {
    arguments[1] = arguments[0];
    var Q = new Array(2);
    for (var i = 0; i < 2; i++)
        Q[i] = arguments[i];
    return Q;
}

var Q = f("a");
Q = f("a");
assertEq(Q[0], "a");

assertEq(Q[1], "a");


is the testcase. It asserts Assertion failed: s0->isQuad() && s1->isQuad() (../nanojit/LIR.cpp:2449) in 64-bit js debug shell, but causes an error instead in 32-bit js shell.

(Only seems to occur when -j is turned on.)

Running autoBisect now on the 32-bit shell.

Comment 9

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

autoBisect shows this is probably related to bug 453730:

The first bad revision is:
changeset:   30020:c76558a87dd9
user:        David Mandelin
date:        Wed Jul 08 11:16:41 2009 -0700
summary:     Bug 453730: trace JSOP_ARGUMENTS, r=gal

Comment 10

[Daniel Veditz [:dveditz]]

Does this affect the 1.9.1 branch? If a regression from bug 453730 I'd guess not.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch for 1.9.2

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #10)

indeed not

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/700e7a9e95ff

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 429553 [details] [diff] [review]
patch for 1.9.2

Approved for 1.9.2.3, a=dveditz for release-drivers

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c3c8827a8767

Comment 16

[Christian Holler (:decoder)]

Bug in removed tracer code, setting in-testsuite- flag.