Assertion failure: [infer failure] Missing type in object [0xf6c00180] length: int, at jsinfer.cpp:325 with rest parameter

VERIFIED FIXED in Firefox 16

Status

()

defect
--
critical
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: decoder, Assigned: efaust)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla17
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14 unaffected, firefox15 unaffected, firefox16 fixed, firefox17 verified, firefox-esr10 unaffected)

Details

(Whiteboard: [jsbugmon:update][advisory-tracking-])

Attachments

(2 attachments)

Reporter

Description

7 years ago
The following test asserts on mozilla-central revision b5ae446888f5 (options -m -n -a):


var Constr = function( ... property)  {};
Constr.prototype = [];
var c = new Constr();
c.push(5);
gc();
function enterFunc() {}
evaluate('enterFunc (c.length);');


S-s due to infer failure, which can be security-critical.
Reporter

Updated

7 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,bisect]
Reporter

Updated

7 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Reporter

Comment 1

7 years ago
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   97629:60fedf46af8e
user:        Eric Faust
date:        Mon Jun 25 17:32:34 2012 -0700
summary:     Bug 764148 - Stop incorrectly shadowing TI properties on sets if a setter would
Reporter

Comment 2

7 years ago
Eric, mind taking a look at this?
Assignee

Comment 3

7 years ago
With Brian's help, was able to determine that the fix to 764148 elided marking certain native properties as own, even when they should have been. Fix on try at https://tbpl.mozilla.org/?tree=Try&rev=586b50d80390
Provisionally calling this sec-critical (inference).
Keywords: sec-critical
Assignee

Comment 5

7 years ago
Posted patch FixSplinter Review
Assignee: general → efaustbmo
Status: NEW → ASSIGNED
Attachment #654299 - Flags: review?(bhackett1024)
Assignee

Comment 6

7 years ago
Attachment #654300 - Flags: review?(bhackett1024)
Comment on attachment 654299 [details] [diff] [review]
Fix

Review of attachment 654299 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/methodjit/PolyIC.cpp
@@ +273,5 @@
>          Label stubShapeJumpLabel = masm.label();
>  
>          pic.setPropLabels().setStubShapeJump(masm, start, stubShapeJumpLabel);
>  
> +        if (pic.typeMonitored || adding) {

The comment below this should be updated to explain why the type guard is necessary in the adding case (we need to make sure that the property is marked as 'own' for objects of different type but the same shape), and to clarify that the existing comment only applies to the pic.typeMonitored case.
Attachment #654299 - Flags: review?(bhackett1024) → review+
Comment on attachment 654300 [details] [diff] [review]
Ionmonkey changes

Review of attachment 654300 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/IonCaches.cpp
@@ +445,5 @@
>      MacroAssembler masm;
>  
>      Label failures;
>  
> +    /* Guard the Type of the object */

s/Type/type/
Attachment #654300 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/455ed4a415aa
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Reporter

Updated

7 years ago
Status: RESOLVED → VERIFIED
Reporter

Comment 11

7 years ago
JSBugMon: This bug has been automatically verified fixed.
Assignee

Comment 12

7 years ago
Comment on attachment 654299 [details] [diff] [review]
Fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 764148
User impact if declined: Potential security risk (Type Inference)
Testing completed (on m-c, etc.): Tested on test in bug.
Risk to taking this patch (and alternatives if risky): Mild. Changes the way inference marks own properties. Alternatives include backing out 764148.
String or UUID changes made by this patch: None.
Attachment #654299 - Flags: approval-mozilla-aurora?
Assignee

Comment 13

7 years ago
Original bug landed to central June 26, so should be also affecting FF 16, I believe.
Comment on attachment 654299 [details] [diff] [review]
Fix

[Triage Comment]
Low risk sec-critical fix for a FF16 regression. Approving for FF16.
Attachment #654299 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 785576
Reporter

Updated

7 years ago
Duplicate of this bug: 785357
Assignee

Comment 16

7 years ago
Comment on attachment 654299 [details] [diff] [review]
Fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 764148
User impact if declined: Sec-critical assertion failure
Testing completed (on m-c, etc.): Tested on patch in this bug, as well as bugs 785576, 785749, 785824, and 789835 as well as successful m-c landing.
Risk to taking this patch (and alternatives if risky): Low. Minor TI change.
String or UUID changes made by this patch: None.

To land as single commit with patch in 785576 applied.
Attachment #654299 - Flags: approval-mozilla-beta?
Attachment #654299 - Flags: approval-mozilla-beta?
Attachment #654299 - Flags: approval-mozilla-beta+
Attachment #654299 - Flags: approval-mozilla-aurora+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][advisory-tracking-]
Group: core-security
Reporter

Comment 18

7 years ago
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.