793805 - Crash [@ js_SuppressDeletedElements]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

try {
    o1 = f1 = m1 = new Object
    b1 = {}
    mjitChunkLimit(7)
    o2 = __proto__
    f1.__iterator__ = function() m1 + 2;
    (function() {
        Object.defineProperty(this, "f", {
            get: function() {
                b1 + this
                f;
            }
        })
    }()(f))
} catch (e) {}
try {
    (function() {
        o2.toString = function() {
            for (p in 2) {
                t.y()
            }
            o1
        }
    }())(f)
} catch (e) {}
try {
    for (v in f1) {}
} catch (e) {}
try {
    f
} catch (e) {}
print(ArrayBuffer())
try {
    f
} catch (e) {}
try {
    o1 = Object
    f
} catch (e) {}
try {
    f
} catch (e) {}
try {
    f
} catch (e) {}
try {
    a1 = [, 0]
    f
} catch (e) {}
gc()
a1.splice(1)

crashes js opt shell on m-c changeset b2867d82dcad without any CLI arguments at js_SuppressDeletedElements.

s-s because gc and ArrayBuffer are involved.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   107460:09c03f14f5f5
user:        Nicholas Nethercote
date:        Sun Sep 16 16:32:02 2012 -0700
summary:     Bug 791611 (part 1) - Exactly root most JSObjects in jsinfer.cpp.  r=terrence.

Comment 1

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

Attachment: Valgrind stack

Julian, on Valgrind SVN r13012 on Mac OS X 10.7.5 I get the following attached system library leak with this testcase, could it be added to the Valgrind suppressions list, assuming it's a system library leak?

Comment 2

[Christian Holler (:decoder)]

Jandem was working on this crash this morning after LangFuzz hit it with an unreliable testcase.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Christian Holler (:decoder) from comment #2)
> Jandem was working on this crash this morning after LangFuzz hit it with an
> unreliable testcase.

Yeah it looked like SuppressDeletedPropertyHelper was accessing a GC'ed PropertyIteratorObject stored in cx->enumerators. This crashes since the NativeIterator stored inside it is freed and set to NULL when finalizing the PropertyIteratorObject.

I couldn't tell more since I was unable to reproduce the crash, but it may be IonMonkey not marking an active iterator or not closing one in some cases (both tests involve over-recursion?).

Comment 4

[Daniel Veditz [:dveditz]]

From the stack this looks like a nullptr deref... is there a security problem here or is the iterator ptr always going to be null when not valid?

Comment 5

[Christian Holler (:decoder)]

(In reply to Daniel Veditz [:dveditz] from comment #4)
> From the stack this looks like a nullptr deref... is there a security
> problem here or is the iterator ptr always going to be null when not valid?

According to comment 3 this could lead to a uaf condition. A live enumerator is GC'ed. It's set to NULL so using it afterwards will cause a NULL deref. But I would assume that if it was GC'ed then it could be reused at some point.

Jandem, can you confirm that? Or is this always a save crash?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Christian Holler (:decoder) from comment #5)
> 
> Jandem, can you confirm that? Or is this always a save crash?

Yeah, PropertyIteratorObject::finalize NULLs the object's private slot, but we could indeed allocate a new object there and then dereference whatever is stored in that slot.

Comment 7

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

Assuming sec-critical based on comment 6.

Comment 8

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

> Assuming sec-critical based on comment 6.

njn, do you mind taking a look? autoBisect points to a checkin that might be related, in comment 0.

Comment 9

[Terrence Cole [:terrence]]

I just tried to repro on linux.  I was unable to trigger a crash under gcc-4.5.4 or clang-3.1 in x86 and x64 opt builds.  This stinks of mis-compilation.

Comment 10

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

(In reply to Terrence Cole [:terrence] from comment #9)
> I just tried to repro on linux.  I was unable to trigger a crash under
> gcc-4.5.4 or clang-3.1 in x86 and x64 opt builds.  This stinks of
> mis-compilation.

You're right. I think I upgraded Clang on Mac in between, and now on Clang 4.1 I'm unable to reproduce.

Are the stacks helpful?

Comment 11

[Andrew McCreight [:mccr8]]

Can we reproduce this problem on the version of Clang we're shipping OSX on?

Comment 12

[Andrew McCreight [:mccr8]]

Err, that we're using to ship the OSX version of Firefox.

Comment 13

[Christian Holler (:decoder)]

The crash I was seeing with this signature was on GCC, not on clang, so I'm quite sure this isn't clang-specific. I was not able to isolate a reliable testcase though, it was behaving non-deterministically. Could be a different bug of course with the same crash signature in the same time window but that would be a big coincidence then.

Comment 14

[Terrence Cole [:terrence]]

(In reply to Christian Holler (:decoder) from comment #13)
> The crash I was seeing with this signature was on GCC, not on clang, so I'm
> quite sure this isn't clang-specific. I was not able to isolate a reliable
> testcase though, it was behaving non-deterministically. Could be a different
> bug of course with the same crash signature in the same time window but that
> would be a big coincidence then.

Is this the GCC 4.2 from older Mac versions?  We've had trouble with it miscompiling Rooted before.

Comment 15

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

> Is this the GCC 4.2 from older Mac versions?  We've had trouble with it
> miscompiling Rooted before.

Yes! I guess what I meant in comment 10 was that I was using GCC 4.2 on Mac until it became necessary to use Clang.

Comment 16

[Christian Holler (:decoder)]

(In reply to Terrence Cole [:terrence] from comment #14)

> 
> Is this the GCC 4.2 from older Mac versions?  We've had trouble with it
> miscompiling Rooted before.

No, it was GCC 4.5.2 on Linux.

Comment 17

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

Attachment: Valgrind stack

Valgrind stack for Julian - to be suppressed. Now comes with Valgrind-suppression entry.

Comment 18

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

> Yes! I guess what I meant in comment 10 was that I was using GCC 4.2 on Mac
> until it became necessary to use Clang.

So I probably mis-spoke. I have it reproducing with specified rev changeset, and an older changeset of our compilation harness. Will hand over to Terrence to take a look.

Comment 19

[Terrence Cole [:terrence]]

I stepped through the splice preceding the crash with Waldo and it appears to be working fine.  This crashes involves cx->enumerators and does not reproduce if you remove the mjitChunkLimit.  This suggests to me that this is a JaegerMonkey bug at the dread crossroads of stack limits, iterators, and chunked compilation.  This is too far afield of my own expertise: bhackett or jandem is going to have to take a look at it to get further.

Comment 20

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

Note to self: be on harness rev 4d1576e33a0f to reproduce this issue with m-c changeset b2867d82dcad on 32-bit js opt shell in Mac 10.7.5, clang 4.1.

This probably got fixed by part 2 of bug 793588. Is this plausible? (if so, testcase should be landed)

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   107990:c9b1f0ce3779
user:        Nicholas Nethercote
date:        Sun Sep 23 20:53:27 2012 -0700
summary:     Bug 793588 (part 2) - Exactly root jsiter.{cpp,h}.  r=sfink.

Comment 21

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

Terrence and I spoke about this. Since it has gone away, there's nothing much we can do about it. When the next cycles comes, the testcase should be landed. In the meantime, resolving FIXED since a possible known fix was discovered in the previous comment.

Comment 22

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

This was really bug 812341.