If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 26

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: gkw, Assigned: sunfish)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
mozilla26
x86_64
Windows 7
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox24 unaffected, firefox25 unaffected, firefox26 fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [fuzzblocker], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Created attachment 800487 [details]
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.
(Reporter)

Comment 1

4 years ago
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.
Blocks: 885169
(Reporter)

Updated

4 years ago
Flags: needinfo?(sunfish)
(Assignee)

Comment 2

4 years ago
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?
Flags: needinfo?(sunfish)
(Assignee)

Comment 3

4 years ago
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?
Flags: needinfo?
(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.
(Reporter)

Comment 5

4 years ago
(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
Flags: needinfo?
(Reporter)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
(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).
(Assignee)

Comment 8

4 years ago
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?
Flags: needinfo?(philringnalda)
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.
Flags: needinfo?(philringnalda)
(Assignee)

Comment 10

4 years ago
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?
Flags: needinfo?
(Assignee)

Comment 11

4 years ago
Of course, I meant the fix from bug 885169, in the above comment.
Flags: needinfo?
Sounds good to me.
(Assignee)

Comment 13

4 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Reporter)

Comment 14

4 years ago
> 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.
Flags: in-testsuite-
Assignee: general → sunfish
status-b2g18: --- → unaffected
status-firefox24: --- → unaffected
status-firefox25: --- → unaffected
status-firefox26: --- → fixed
status-firefox-esr17: --- → unaffected
Target Milestone: --- → mozilla26
Group: core-security
You need to log in before you can comment on or make changes to this bug.