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)

x86_64
Linux
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla22
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
firefox-esr17 --- wontfix
b2g18 --- wontfix
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: decoder, Assigned: nmatsakis)

Details

(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update,bisect,testComment=1,origRev=f99a075a5bce])

Attachments

(1 file)

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);
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
Marking s-s due to length/range assertion violation on trunk.
Group: core-security
Whiteboard: [jsbugmon:ignore] → [jsbugmon:update,bisect]
Version: Other Branch → Trunk
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect,testComment=1,origRev=f99a075a5bce]
Assignee: general → nmatsakis
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.
Is there any security risk caused by the assertion?
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.
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?
Keywords: sec-high
Ah, I see, you are correct.  Most likely this affects earlier branches!  Sorry for the delay, I'll prepare a patch today.
Should I add the test case into the auto-regress directory, or is that done through some other process?
(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.
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.
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).
Affects all our releases based on comment 13. I really don't know why the fuzzers didn't find this at all earlier.
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)
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.
https://hg.mozilla.org/mozilla-central/rev/df0fe541737e
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Please prepare nominations for Aurora/Beta/ESR17 when you get the chance. Thanks!
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
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?
Niko, please fill out the approval request comment.
Flags: needinfo?(nmatsakis)
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)
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?
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.
Flags: needinfo?(luke)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: