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

RESOLVED FIXED in mozilla1.9.3a4

Status

()

--
critical
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: marcia, Assigned: dvander)

Tracking

(5 keywords)

Trunk
mozilla1.9.3a4
assertion, crash, crashreportid, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(status1.9.2 .4-fixed, status1.9.1 unaffected)

Details

(Whiteboard: [sg:dos] maybe worse on x86-64? [fixed-in-tracemonkey], crash signature, URL)

Attachments

(2 attachments)

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.
Confirmed on Windows Vista as well:

http://crash-stats.mozilla.com/report/index/9cad21ae-df8f-4fd7-bac0-d34502100219
Keywords: crashreportid
OS: Mac OS X → All
Hardware: x86 → All
Also latest Namoroko 3.6 crashes on this site, without report here.
STR: move your mouse over the "Travel Information" on the left side.
(Reporter)

Updated

9 years ago
Summary: Crash in [@ GetPropertyByName ] when loading Trailways site → Crash in [@ GetPropertyByName ] when mousing over Travel information on Trailways site
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).
Group: core-security
Created attachment 427877 [details] [diff] [review]
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).
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #427877 - Flags: review?(dmandelin)
blocking1.9.2: --- → ?
Attachment #427877 - Flags: review?(dmandelin) → review+
Whiteboard: [sg:dos] maybe worse on x86-64?
http://hg.mozilla.org/tracemonkey/rev/700e7a9e95ff

leaving test case out until bug is opened.
Whiteboard: [sg:dos] maybe worse on x86-64? → [sg:dos] maybe worse on x86-64? [fixed-in-tracemonkey]
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.
Keywords: assertion, regression, testcase
Summary: Crash in [@ GetPropertyByName ] when mousing over Travel information on Trailways site → TM: Crash in [@ GetPropertyByName ] when mousing over Travel information on Trailways site, or "Assertion failed: s0->isQuad() && s1->isQuad() (../nanojit/LIR.cpp"
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
Blocks: 453730
Does this affect the 1.9.1 branch? If a regression from bug 453730 I'd guess not.
blocking1.9.2: ? → ---
status1.9.2: --- → wanted
Created attachment 429553 [details] [diff] [review]
patch for 1.9.2
Attachment #429553 - Flags: review?(dmandelin)
Attachment #429553 - Flags: approval1.9.2.2?
(In reply to comment #10)

indeed not
status1.9.1: --- → unaffected
Attachment #429553 - Flags: review?(dmandelin) → review+

Comment 13

9 years ago
http://hg.mozilla.org/mozilla-central/rev/700e7a9e95ff
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Attachment #429553 - Flags: approval1.9.2.2? → approval1.9.2.3?
Attachment #429553 - Flags: approval1.9.2.3? → approval1.9.2.3+
Comment on attachment 429553 [details] [diff] [review]
patch for 1.9.2

Approved for 1.9.2.3, a=dveditz for release-drivers
Blocks: 554670
status1.9.2: wanted → .4-fixed
Target Milestone: --- → mozilla1.9.3a4
Group: core-security
Group: core-security
Group: core-security
Crash Signature: [@ GetPropertyByName ]
Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.