1066659 - Crash [@ getClass] with poisoned pointer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 2db5b64f6d49 (run with --fuzzing-safe --thread-count=2 --ion-regalloc=backtracking):


var VERBOSE = false;
function toPrinted(value) {}
function reportCompare (expected, actual) {
  var expected_t = typeof expected;
  var actual_t = typeof actual;
  var output = "";
  if (expected_t != actual_t) 
    output += "x"+ actual_t + " ";
  else if (VERBOSE) {}
  expected != actual;
  output += toPrinted(expected);
}
function f(x) {
    x = eval("a = arguments.callee.arguments;");
}
gczeal(7,1);
for (var i=0; i < 1000 ; i++) {
    reportCompare("new Date()", function(x) expect(Date.prototype.summary.call(x)))
    f(5);
    reportCompare(a[0], 10);
}

Comment 1

[Christian Holler (:decoder)]

Crash trace:

Program received signal SIGSEGV, Segmentation fault.
0x086688cd in getClass (this=0xf5509ce0) at js/src/vm/PosixNSPR.cpp:358
358     }
#0  0x086688cd in getClass (this=0xf5509ce0) at js/src/vm/PosixNSPR.cpp:358
#1  is<js::StringObject> (this=(const JSObject * const) 0xf5509ce0 Cannot access memory at address 0x2b2b2b2b) at js/src/jsobj.h:1165
#2  js::ToPrimitive (cx=0x95cfe40, vp=$jsval(-nan(0xfff88f5509ce0))) at js/src/jsobjinlines.h:861
#3  0x08668c8e in js::LooselyEqual (cx=0x95cfe40, lval=..., rval=..., result=0xffffbf60) at js/src/vm/Interpreter.cpp:733
#4  0x084ac107 in js::jit::LooselyEqual<false> (cx=0x95cfe40, lhs=$jsval(-nan(0xfff85f7023ee0)), rhs=$jsval(-nan(0xfff88f5509ce0)), res=0xffffbf60) at js/src/jit/VMFunctions.cpp:238
#5  0xf7fccfa6 in ?? ()
eax     0x2b2b2b2b      724249387
eip     0x86688cd <js::ToPrimitive(JSContext*, JS::MutableHandleValue)+77>
=> 0x86688cd <js::ToPrimitive(JSContext*, JS::MutableHandleValue)+77>:  mov    (%eax),%eax
   0x86688cf <js::ToPrimitive(JSContext*, JS::MutableHandleValue)+79>:  mov    %eax,-0x44(%ebp)


Marking sec-critical due to poisoned pointer involved.

Comment 2

[Christian Holler (:decoder)]

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

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
Due to skipped revisions, the first bad revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a0dd5a83ba36
user:        Jan de Mooij
date:        Thu Jul 24 11:56:43 2014 +0200
summary:     Bug 1031529 part 2 - Remove JS_THREADSAFE #ifdefs everywhere. r=bhackett

changeset:   https://hg.mozilla.org/mozilla-central/rev/6426fef52f51
user:        Jan de Mooij
date:        Thu Jul 24 11:56:45 2014 +0200
summary:     Bug 1031529 part 3 - Step defining JS_THREADSAFE, remove --disable-threadsafe. r=glandium

This iteration took 0.640 seconds to run.

Comment 4

[Christian Holler (:decoder)]

I guess this goes back beyond the --disable-threadsafe removal. Gary, could you bisect this manually? Thanks!

Comment 5

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

I tried to reproduce this many times on Mac and Linux but to no avail, so I guess this has to go on to Jan. Jan, do you think you might be able to find out the issue here?

Comment 6

[Christian Holler (:decoder)]

I think it's a dup of bug 1065362 but that one also doesn't have a bisect yet.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] GMT+8 Sep 4 - 23 from comment #5)
> I tried to reproduce this many times on Mac and Linux but to no avail, so I
> guess this has to go on to Jan. Jan, do you think you might be able to find
> out the issue here?

For me it reproduces reliably on OS X debug 32-bit with the flags in comment 0. Investigating.

Comment 8

[Jan de Mooij [:jandem]]

This is a 32-bit only problem with the backtracking allocator and safepoints, though I'm not sure the underlying bug is backtracking-only.

We have a safepoint for an LConcat where an argument payload is in a register (ebp) and the type is still in the argument slot. populateSafepoints correctly calls addNunboxPayload, but then in SafepointWriter::writeNunboxParts we take this branch:

    if (entry.type.isUse() || entry.payload.isUse()) {
        partials--;
        continue;
    }

This means we forget about the payload in ebp and don't update it on moving GC. Then later on we pass ebp to a VM call and crash.

Slightly smaller testcase that fails with --no-threads --ion-regalloc=backtracking on OS X, 32-bit:

function reportCompare (expected, actual) {
    var actual_t = typeof actual;
    var output = "";
    output += "x" + actual_t + " ";
    expected != actual;
    output += undefined;
}
gczeal(7,1);
for (var i=0; i<900; i++) {
    reportCompare("abc", function() {});
    reportCompare(undefined, 10);
}

Comment 9

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

Thanks, Jan, those pieces of information were extremely useful, and I've managed to get a bisection:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/0f85fbed35dc
user:        Hannes Verschore
date:        Fri Jul 04 19:43:24 2014 +0200
summary:     Bug 1019983 - Don't optimize compare based on baseline caches when more types are seen than present in the cache, r=jandem

Comment 10

[Hannes Verschore [:h4writer]]

This is not caused by bug 1019983. That bug only disallows some specialized LIR on certain occasions. (MCompare becomes LCompareVM instead of ?LCompareV?).

Adjusting the test to the following triggers the segfault before this changeset,
(--ion-parallel-compile=off --ion-eager --ion-inlining=off --ion-regalloc=backtracking)

> function reportCompare (expected, actual) {
>         var actual_t = typeof actual;
>             var output = "";
>                 output += "x" + actual_t + " ";
>                     expected != actual;
>                         output += undefined;
> }
> gczeal(7,1);
> for (var i=0; i<900; i++) {
>         reportCompare("abc", function() {});
>         reportCompare(null, 10);
> }



changeset:   183175:8c234572141a
user:        Terrence Cole <[e-mail]>
date:        Mon May 05 17:10:29 2014 -0700
summary:     Bug 989414 - Always allocate lambda objects in the nursery; r=jonco

Comment 12

[Terrence Cole [:terrence]]

This repros on tip, in linux.

Configure:
CXX="g++ -m32" CC="gcc -m32" AR="ar" ./configure --target=i686-linux-gnu --enable-optimize --enable-debug --without-intl-api --disable-ctypes

Run:
./dist/bin/js --ion-offthread-compile=off --ion-eager --ion-inlining=off --ion-regalloc=backtracking bug-1066659.js

The pointer-to-swept-nursery is coming off of the jit stack and into a Value in/out parameter to LooselyEqual. I'd guess this is almost certainly bug 1023158. I had been putting off bug 1023158 because if it was /really/ a problem we should be seeing fuzz bugs that look exactly like this. Guess it's time to re-prioritize!

I'm going to try to fix bug 1023158 first, as that should give us an immediate, gratifying assertion right at the failure.

Comment 13

[Jan de Mooij [:jandem]]

See also the analysis in comment 8; I still think this is a regalloc bug.

Comment 14

[Terrence Cole [:terrence]]

Also, this is a live pointer, so bug 1023158 isn't actually applicable.

Comment 15

[Terrence Cole [:terrence]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> See also the analysis in comment 8; I still think this is a regalloc bug.

The spilled Value should be getting marked as part of MarkRuntime, which should be seeded by safepoints by the gc. So it seems that you are correct: this probably is a regalloc bug.

Comment 16

[Terrence Cole [:terrence]]

Oops. s/by the gc/by the regalloc for the gc/

Comment 17

[Terrence Cole [:terrence]]

Jan and I discussed this on IRC last week and I concur: this is most likely a RegAlloc bug. Jan is this still on your radar?

Comment 18

[Andrew McCreight [:mccr8]]

I emailed Brian and he said he'd look at it soon.  Just unassign yourself if you can't look at it, and we'll find somebody else.  Thanks.

Comment 19

[Brian Hackett [Laid off!]]

Attachment: patch

Thanks for zeroing in on the problem in comment 8.  This is, yeah, a bug with the backtracking allocator and safepoints, and only affects the backtracking allocator.  The backtracking allocator allocates the type and payload vregs on NUNBOX32 platforms independently, so the two are added to safepoints at different points and we end up with these partial nunbox entries.  LSRA adds the type and payload allocations together.  Since we currently only use the backtracking allocator for asm.js code, where there are no safepoints, this shouldn't be a security issue anywhere.

The attached patch fixes this by looking through all the nunbox entries for an allocation for the type, in cases where the payload has an allocation but not the type.

Comment 21

[Jan de Mooij [:jandem]]

Comment on attachment 8510356 [details] [diff] [review]
patch

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

::: js/src/jit/CompactBuffer.h
@@ +129,5 @@
>              writeByte(byte);
>              value >>= 7;
>          } while (value);
>      }
> +    void writeUnsignedAt(uint32_t pos, uint32_t value, uint32_t original) {

Assert value <= original? If that doesn't hold the loop below may not write the full value, causing weird bugs.

::: js/src/jit/Safepoints.cpp
@@ +316,5 @@
> +            // No allocation associated with the type. Look for another
> +            // safepoint entry with an allocation for the type.
> +            entry.type = safepoint->findTypeAllocation(entry.typeVreg);
> +            if (entry.type.isUse())
> +                continue;

Can this happen?

Comment 22

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91ac49c348b1

Comment 23

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #21)
> ::: js/src/jit/Safepoints.cpp
> @@ +316,5 @@
> > +            // No allocation associated with the type. Look for another
> > +            // safepoint entry with an allocation for the type.
> > +            entry.type = safepoint->findTypeAllocation(entry.typeVreg);
> > +            if (entry.type.isUse())
> > +                continue;
> 
> Can this happen?

Yes, if the payload is spilled by the backtracking allocator but the type isn't, the payload can still end up by itself in safepoints after the type is dead.

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/91ac49c348b1