730888 - Crash [@ JSString::isLinear]

Status: VERIFIED FIXED

Description

[Christian Holler (:decoder)]

The following test crashes on mozilla-central revision 2dc40eb83023 (options -m -n):


(function() {
    for (var i = 0; i < 64; ++i) {
        var name;
        switch (this) {
            case 0: name = 'firstAttr'; break;
            case 1: name = 'secondAttr'; 
            case 2: name = 'thirdAttr'; break;
        }
        switch (name) {
          case 'firstAttr': assertEq(result, 'value'); break;
        }
    }
})();


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000442e8c in JSString::isLinear (this=0x0) at /srv/repos/mozilla-central/js/src/vm/String.h:316
316             return (d.lengthAndFlags & LINEAR_MASK) == LINEAR_FLAGS;
(gdb) bt
#0  0x0000000000442e8c in JSString::isLinear (this=0x0) at /srv/repos/mozilla-central/js/src/vm/String.h:316
#1  0x0000000000443232 in JSString::ensureLinear (this=0x0, cx=0xb62b40) at /srv/repos/mozilla-central/js/src/vm/String.h:794
#2  0x0000000000787540 in js::mjit::stubs::LookupSwitch (f=..., pc=0xb6859f "\001") at /srv/repos/mozilla-central/js/src/methodjit/StubCalls.cpp:1517
#3  0x00007ffff7f3c464 in ?? ()
#4  0x00007ffff7f3c078 in ?? ()
#5  0x0000000000000000 in ?? ()



Bisect:

The first bad revision is:
changeset:   87569:8ab9fea628bd
parent:      87565:f077e2e7e38d
user:        Brian Hackett
date:        Thu Feb 23 13:01:27 2012 -0800
summary:     Efficiency improvements in ScriptAnalysis::analyzeSSA, bug 725920. r=dvander


I'm locking this because I also have a very close testcase that causes an infer failure (where bisect also points to this changeset), so I cannot be sure that this is harmless.

Comment 1

[Daniel Veditz [:dveditz]]

(In reply to Christian Holler (:decoder) from comment #0)
> I'm locking this because I also have a very close testcase that causes an
> infer failure (where bisect also points to this changeset), so I cannot be
> sure that this is harmless.

Could coincidentally be a completely different bug. Or not. either way we should capture it either by putting that testcase in this bug (and bhackett can tell us if it's the same thing) or filing as separate bug (which can later be duped if appropriate).

This crash looks null deref-ish on the surface. the other testcase may be more interesting

Comment 2

[Christian Holler (:decoder)]

(In reply to Daniel Veditz [:dveditz] from comment #1)

> Could coincidentally be a completely different bug. Or not. either way we
> should capture it either by putting that testcase in this bug (and bhackett
> can tell us if it's the same thing) or filing as separate bug (which can
> later be duped if appropriate).

I usually keep these bugs queued and retest them once the other bug is fixed to avoid missing any of them.

> This crash looks null deref-ish on the surface. the other testcase may be
> more interesting

I'll post the other one if this one doesn't help bhackett to locate the problem. Since the other is an infer failure, it won't tell you much about the security impact either.

Comment 3

[Brian Hackett (:bhackett)]

Attachment: back out switch consolidation

Bug 725920 added two optimizations to SSA analysis, one for consolidating arrays of pending values for switch statements and one for avoiding excess crawling of these pending arrays for all branches.  The former breaks some SSA invariants here that the offsets for phi nodes corresponds to the place they were actually formed (leading to cycles where not all values are accounted for).  This patch just backs that optimization out --- the second one is cleaner and more general, and the former shouldn't make a difference for performance in either asymptotic behavior or constant factors.

Comment 4

[Brian Hackett (:bhackett)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbd0cb4228d7

Comment 5

[Brian Hackett (:bhackett)]

https://hg.mozilla.org/mozilla-central/rev/fbd0cb4228d7