Assertion failure: [infer failure] Missing type in object [0x7ff9cfa00250] (index): string, at jsinfer.cpp:326

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: Benjamin)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla16
x86_64
Linux
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +
in-litmus -
in-moztrap -

Firefox Tracking Flags

(firefox15-)

Details

(Whiteboard: js-triage-needed)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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


var patterns = new Array(); 
patterns[0] = '';
  for (i in patterns)
    s = patterns[i];
    status =(function  ( ... patterns  )  {
    patterns[i];
  }    ) (s);


This is the first bug found with the new grammar supporting Harmony's rest parameters (bug 574132), so it might be a regression from that bug. S-s because infer failures can be security-critical.
(Assignee)

Comment 1

5 years ago
Created attachment 632336 [details] [diff] [review]
add integer properties to rest argument
Assignee: general → bpeterson
Attachment #632336 - Flags: review?(bhackett1024)
Comment on attachment 632336 [details] [diff] [review]
add integer properties to rest argument

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

::: js/src/jsinfer.cpp
@@ +3657,5 @@
>                  return false;
>              types->addType(cx, Type::ObjectType(rest));
> +
> +            // Simulate set element.
> +            rest->getProperty(cx, JSID_VOID, true)->addType(cx, Type::UnknownType());

The getProperty() return value needs a NULL check.
Attachment #632336 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 633299 [details] [diff] [review]
add null check
Attachment #632336 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Attachment #633299 - Attachment description: add type check → add null check
(In reply to Christian Holler (:decoder) from comment #0)
> This is the first bug found with the new grammar supporting Harmony's rest
> parameters (bug 574132) so it might be a regression from that bug.

Setting tracking-firefox15=? -- if this is indeed a regression from bug 574132, then Firefox 15 would presumably be affected & need a backport.

> S-s because infer failures can be security-critical.

I was about to land the attached checkin-needed patch, and then I noticed that it includes a testcase.

Benjamin / Brian: If this is actually a security-sensitive bug, should we hold off on landing that testcase?   (Normally we wait to land testcases for security-sensitive bugs, until after users have gotten the fix on all affected branches.)

Or alternately: maybe this bug didn't actually turn out to be scary from a security perspective? (in which case we can open it up?)
tracking-firefox15: --- → ?
Keywords: checkin-needed
(meant to say: removing checkin-needed keyword, pending an answer to the end of comment 4, so that another patch-lander doesn't come along and land the patch, maybe publishing a potentially-scary testcase while we've still got users running vulnerable builds)
(Assignee)

Comment 6

5 years ago
This bug is not a security bug. On release builds, the test passes successfully.
(Reporter)

Comment 7

5 years ago
(In reply to Benjamin Peterson from comment #6)
> This bug is not a security bug. On release builds, the test passes
> successfully.

The fact that this test does not crash on release builds does not mean that it is not a security bug. We previously had infer failures that did not crash on release builds, but still those we're vulnerable too (a modified test exposed a crash on the release build as well).
(Assignee)

Comment 8

5 years ago
This particular test case is not exploitable.
(Reporter)

Comment 9

5 years ago
(In reply to Benjamin Peterson from comment #8)
> This particular test case is not exploitable.

Removing s-s then :)
Group: core-security
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b589776a7912
Keywords: checkin-needed
Flags: in-testsuite+
Flags: in-moztrap-
Flags: in-litmus-
https://hg.mozilla.org/mozilla-central/rev/b589776a7912
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Does this affect 15 and need to be uplifted? If so, please go ahead and nominate for landing approval.
Not s-s, so no particular reason to track.
tracking-firefox15: ? → -
You need to log in before you can comment on or make changes to this bug.