913272 - Crash [@ js::GetPropertyHelper] or [@ js::IsInRequest]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: debug and opt stacks

function f(x) {
    for (var i = 0; i < x.length; ++i) {
        for (var j = 0; j < x[i].w; ++j) {}
    }
}
f([]);
f([{
    w: 5
}]);

crashes js 64-bit opt shell on m-c changeset df8f342e9a6b without any CLI arguments at js::GetPropertyHelper and crashes js debug shell at js::IsInRequest

My configure flags are:

--host=x86_64-pc-mingw32 --target=x86_64-pc-mingw32 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-threadsafe <other NSPR options>

s-s because there seemingly are some memory addresses on the stack. This is a fuzzblocker because it is reduced from jsfunfuzz code itself, without any generation of random code.

Running autoBisect now.

Comment 1

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/997672af6fc8
user:        Dan Gohman
date:        Wed Sep 04 21:16:07 2013 -0700
summary:     Bug 885169 - Reverse the default register allocation order so that low registers like eax on x86/x64 are preferred over high registers. r=h4writer

Dan, you might have a fix for this Win64 bustage in bug 885169, but in case you need a testcase, here it is.

Comment 2

[Dan Gohman [:sunfish]]

As described in bug 885169, 997672af6fc8 is already reverted from mozilla-central, though it appears to still be in mozilla-inbound. What should I do here? Should I ask for it to be reverted from mozilla-inbound?

Also, I don't presently have a Win64 box to test this on. Could you or someone else determine whether the fix posted to bug 885169 fixes the testcase here?

Comment 3

[Dan Gohman [:sunfish]]

Also, remarks in bug 885169 suggest that the underlying bug may be visible on Win64 even without 997672af6fc8. If so, it's likely very security-sensitive. Should bug 885169 be made security-sensitive? Should I seek sec-approval for the fix before checking it in anywhere?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

(In reply to Dan Gohman [:sunfish] from comment #2)
> As described in bug 885169, 997672af6fc8 is already reverted from
> mozilla-central, though it appears to still be in mozilla-inbound. What
> should I do here? Should I ask for it to be reverted from mozilla-inbound?

Any that lands on m-c will eventually land on m-i via the branch merges we do daily.

Comment 5

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

(In reply to Dan Gohman [:sunfish] from comment #3)
> Also, remarks in bug 885169 suggest that the underlying bug may be visible
> on Win64 even without 997672af6fc8. If so, it's likely very
> security-sensitive. Should bug 885169 be made security-sensitive? Should I
> seek sec-approval for the fix before checking it in anywhere?

If it's m-c only, sec-approval is not required:

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 6

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

(In reply to Dan Gohman [:sunfish] from comment #2)
> Also, I don't presently have a Win64 box to test this on. Could you or
> someone else determine whether the fix posted to bug 885169 fixes the
> testcase here?

Yes, the fix posted to that bug does fix this testcase.

Comment 7

[Dan Gohman [:sunfish]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #5)
> (In reply to Dan Gohman [:sunfish] from comment #3)
> > Also, remarks in bug 885169 suggest that the underlying bug may be visible
> > on Win64 even without 997672af6fc8. If so, it's likely very
> > security-sensitive. Should bug 885169 be made security-sensitive? Should I
> > seek sec-approval for the fix before checking it in anywhere?
> 
> If it's m-c only, sec-approval is not required:
> 
> https://wiki.mozilla.org/Security/Bug_Approval_Process

If this bug is observable without 997672af6fc8, then it looks like it may have been introduced in f6e861adb467 (bug 846111), meaning several release branches would be affected. If Win64 is considered released, that is (I have no idea what the status is).

Comment 8

[Dan Gohman [:sunfish]]

Philor, your remarks in bug 885169 suggest that the fix posted to that testcase fixed more than just the regressions caused by 997672af6fc8. If that's true, it suggests that all releases on Win64 back to f6e861adb467 (bug 846111) could contain a severe security bug. Can you confirm this?

Comment 9

[Phil Ringnalda (:philor)]

Insufficient data. https://tbpl.mozilla.org/?tree=Date&rev=d35342e7bcd0 is a typical full set of Win64 tests, including four (random, not always the same) suites that crashed [@ nsStandardURL::Release()] coming out of image decoding. Your non-full set on try, before I just retriggered a bunch, had zero of those crashes. Did you happen to fix that crash, or was it just coincidence, or are the suites that you didn't run on try much more prone to hitting it and thus it's both not-fixed and not-coincidence?

But there's no sense in which Win64 is released - we don't build it anywhere but on the trunk, we don't particularly advertise it there. Despite that fact, we have around 45% of our Windows trunk nightlies users on it, thus my urgency about not shipping a nightly that crashes on startup, but there's no release urgency, just 43,000 people using an unsupported nightly.

Comment 10

[Dan Gohman [:sunfish]]

Ok, thanks for info! Given that, I'd like to just check in the fix from bug 885169 without any further deliberation. I'd also like to re-commit 997672af6fc8, though I'll do a full try run on that first. Does that all seem reasonable?

Comment 11

[Dan Gohman [:sunfish]]

Of course, I meant the fix from bug 885169, in the above comment.

Comment 12

[Phil Ringnalda (:philor)]

Sounds good to me.

Comment 13

[Dan Gohman [:sunfish]]

The bug-fix patches are now checked in:

https://hg.mozilla.org/mozilla-central/rev/d6b0a8afb467
https://hg.mozilla.org/mozilla-central/rev/26006e5cefdc
https://hg.mozilla.org/mozilla-central/rev/e2be9740720f

Since Gary confirmed in comment 6 that the third fix fixes the testcase here, and Phil confirmed in comment 9 that we don't need to backport a Win64 fix to any release branches, I'm calling this fixed.

I didn't check in the testcase here; since it's Win64-only and since the Win64 testers found many other regressions with 997672af6fc8 enabled without the fix in d6b0a8afb467, this bug seems pretty well covered already.

Of course, feel free to correct me if any of the above is incorrect.

Comment 14

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

> I didn't check in the testcase here; since it's Win64-only and since the
> Win64 testers found many other regressions with 997672af6fc8 enabled without
> the fix in d6b0a8afb467, this bug seems pretty well covered already.

Yup, moreover this testcase is part of jsfunfuzz code itself, so any regression (of the same testcase) can immediately be found by jsfunfuzz again.