Closed
Bug 847387
Opened 11 years ago
Closed 11 years ago
Assertion failure: argc <= StackSpace::ARGS_LENGTH_MAX, at vm/Stack.cpp:987
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
People
(Reporter: decoder, Assigned: nmatsakis)
Details
(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update,bisect,testComment=1,origRev=f99a075a5bce])
Attachments
(1 file)
853 bytes,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
The following testcase asserts on odinmonkey revision e16905821e73 (run with --ion-eager): var TOTAL_MEMORY = 16777216; var buffer = new ArrayBuffer(TOTAL_MEMORY); var asm = (function(global, env, buffer) { var HEAP32 = new global.Int32Array(buffer); var p = new ParallelArray(HEAP32, function () { return 0; }); })({ Int32Array: Int32Array }, {}, buffer);
Comment 1•11 years ago
|
||
This reproduces on trunk; looks like a ParallelArray bug got caught in the nets :) Here's a shorter testcase: var HEAP32 = new Int32Array(16777216); var p = new ParallelArray(HEAP32, function () { return 0; });
No longer blocks: odinfuzz
Summary: OdinMonkey: Assertion failure: argc <= StackSpace::ARGS_LENGTH_MAX, at vm/Stack.cpp:987 → Assertion failure: argc <= StackSpace::ARGS_LENGTH_MAX, at vm/Stack.cpp:987
Reporter | ||
Comment 2•11 years ago
|
||
Marking s-s due to length/range assertion violation on trunk.
Group: core-security
Reporter | ||
Updated•11 years ago
|
Whiteboard: [jsbugmon:ignore] → [jsbugmon:update,bisect]
Version: Other Branch → Trunk
Reporter | ||
Updated•11 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect,testComment=1,origRev=f99a075a5bce]
Assignee | ||
Updated•11 years ago
|
Assignee: general → nmatsakis
Assignee | ||
Comment 3•11 years ago
|
||
This no longer reproduces under the self-hosted version (bug 829602). Bug 829602 has landed on inbound but has not yet been promoted to trunk.
Reporter | ||
Comment 4•11 years ago
|
||
Is there any security risk caused by the assertion?
Assignee | ||
Comment 5•11 years ago
|
||
Note that bug 829602 was just backed out (though I hope to reintroduce it soon once I've gotten to the bottom of the test failure). As for the assertion, I do not know for certain but I could imagine there would be. I can introduce a simple patch into the trunk for the time being.
Reporter | ||
Comment 6•11 years ago
|
||
I am asking because we might need to backport the patch appropriately to fix branches if they are affected and there is a security risk. Or is this on central only right now?
Assignee | ||
Comment 7•11 years ago
|
||
Ah, I see, you are correct. Most likely this affects earlier branches! Sorry for the delay, I'll prepare a patch today.
Assignee | ||
Comment 9•11 years ago
|
||
Should I add the test case into the auto-regress directory, or is that done through some other process?
Reporter | ||
Comment 10•11 years ago
|
||
(In reply to Niko Matsakis [:nmatsakis] from comment #9) > Should I add the test case into the auto-regress directory, or is that done > through some other process? Just add the test as a normal test, the auto-regress directory was only made for automatically mined tests. However, if this is a security problem that affects released versions, then landing the test should be delayed until the fix is on all affected branches.
Assignee | ||
Comment 11•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/df0fe541737e
Assignee | ||
Comment 12•11 years ago
|
||
No test case is landed. I don't fully understand why the test should be delayed but I have done so. I will investigate which branches are affected now.
Assignee | ||
Comment 13•11 years ago
|
||
OK, so, I landed this patch in ignorance of the proper security policy. This is unfortunate. I believe the vulnerability has always been present since this code was first landed on August 16, 2012 (changeset 06dae3ea0f89).
Reporter | ||
Comment 14•11 years ago
|
||
Affects all our releases based on comment 13. I really don't know why the fuzzers didn't find this at all earlier.
status-firefox19:
--- → affected
status-firefox20:
--- → affected
status-firefox21:
--- → affected
status-firefox22:
--- → affected
status-firefox-esr17:
--- → affected
tracking-firefox19:
--- → ?
tracking-firefox20:
--- → ?
tracking-firefox21:
--- → ?
tracking-firefox22:
--- → ?
tracking-firefox-esr17:
--- → ?
Updated•11 years ago
|
status-b2g18:
--- → affected
status-firefox19:
affected → ---
tracking-b2g18:
--- → ?
tracking-firefox19:
? → ---
Keywords: sec-high → sec-critical
Updated•11 years ago
|
status-firefox19:
--- → wontfix
Assignee | ||
Comment 15•11 years ago
|
||
Based on my reading of the code, even without this patch, the assertion will not trigger a security vulnerability. Rather, it will result in a big chunk of stack being allocated and a "too much recursion" error. Experiments confirmed this hypothesis. However, I would appreciate a second read from someone else familiar with the bug. I am asking Luke for confirmation since he is a Real JavaScript Engineer and he's already involved with this bug, even if only tangentially (but it could be anyone, really). :)
Flags: needinfo?(luke)
Assignee | ||
Comment 16•11 years ago
|
||
More info: what happens here is that we attempt to push a very large number of arguments far in excess of the maximum. Basically one per the length of the array.
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/df0fe541737e
Status: NEW → RESOLVED
Closed: 11 years ago
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → affected
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment 18•11 years ago
|
||
Please prepare nominations for Aurora/Beta/ESR17 when you get the chance. Thanks!
Updated•11 years ago
|
Reporter | ||
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 19•11 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Assignee | ||
Comment 20•11 years ago
|
||
Comment on attachment 721835 [details] [diff] [review] Do not allow more dimensions that we allow arguments. NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed: Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch: [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch: [Approval Request Comment] Regression caused by (bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch: [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: Fix Landed on Version: Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch: See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #721835 -
Flags: approval-mozilla-release?
Attachment #721835 -
Flags: approval-mozilla-esr17?
Attachment #721835 -
Flags: approval-mozilla-beta?
Attachment #721835 -
Flags: approval-mozilla-b2g18?
Attachment #721835 -
Flags: approval-mozilla-aurora?
Comment 21•11 years ago
|
||
Niko, please fill out the approval request comment.
Flags: needinfo?(nmatsakis)
Assignee | ||
Comment 22•11 years ago
|
||
My apologies, I neglected to fill out the [Approval Request] details. Here is the full details: Bug caused by (feature/regressing bug #): bug 778559 User impact if declined: Possible security vulnerability. Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): Minimal to zero. String or UUID changes made by this patch: None
Flags: needinfo?(nmatsakis)
Assignee | ||
Comment 23•11 years ago
|
||
Comment on attachment 721835 [details] [diff] [review] Do not allow more dimensions that we allow arguments. Clearing the various approval bits as I do not believe this is security critical.
Attachment #721835 -
Flags: approval-mozilla-release?
Attachment #721835 -
Flags: approval-mozilla-esr17?
Attachment #721835 -
Flags: approval-mozilla-beta?
Attachment #721835 -
Flags: approval-mozilla-b2g18?
Attachment #721835 -
Flags: approval-mozilla-aurora?
Reporter | ||
Comment 24•11 years ago
|
||
Confirmed with Niko (who also confirmed with :shu) that this is in fact not a security problem, so we can open this up and skip it for the branches.
Group: core-security
tracking-b2g18:
+ → ---
tracking-firefox20:
+ → ---
tracking-firefox21:
+ → ---
tracking-firefox22:
+ → ---
Keywords: sec-critical
Reporter | ||
Updated•11 years ago
|
tracking-firefox-esr17:
20+ → ---
Updated•11 years ago
|
Flags: needinfo?(luke)
You need to log in
before you can comment on or make changes to this bug.
Description
•