Last Comment Bug 763954 - Assertion failure: [infer failure] Missing type in object [0x7ff9cfa00250] (index): string, at jsinfer.cpp:326
: Assertion failure: [infer failure] Missing type in object [0x7ff9cfa00250] (i...
Status: RESOLVED FIXED
js-triage-needed
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla16
Assigned To: :Benjamin Peterson
:
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2012-06-12 08:24 PDT by Christian Holler (:decoder)
Modified: 2012-06-18 18:29 PDT (History)
9 users (show)
geoff: in‑testsuite+
geoff: in‑litmus-
geoff: in‑moztrap-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
add integer properties to rest argument (1.65 KB, patch)
2012-06-12 11:26 PDT, :Benjamin Peterson
bhackett1024: review+
Details | Diff | Splinter Review
add null check (1.77 KB, patch)
2012-06-14 15:18 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-06-12 08:24:44 PDT
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.
Comment 1 :Benjamin Peterson 2012-06-12 11:26:32 PDT
Created attachment 632336 [details] [diff] [review]
add integer properties to rest argument
Comment 2 Brian Hackett (:bhackett) 2012-06-14 15:03:47 PDT
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.
Comment 3 :Benjamin Peterson 2012-06-14 15:18:18 PDT
Created attachment 633299 [details] [diff] [review]
add null check
Comment 4 Daniel Holbert [:dholbert] 2012-06-14 15:37:03 PDT
(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?)
Comment 5 Daniel Holbert [:dholbert] 2012-06-14 15:38:54 PDT
(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)
Comment 6 :Benjamin Peterson 2012-06-14 22:03:25 PDT
This bug is not a security bug. On release builds, the test passes successfully.
Comment 7 Christian Holler (:decoder) 2012-06-15 02:22:05 PDT
(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).
Comment 8 :Benjamin Peterson 2012-06-15 11:28:14 PDT
This particular test case is not exploitable.
Comment 9 Christian Holler (:decoder) 2012-06-15 11:41:58 PDT
(In reply to Benjamin Peterson from comment #8)
> This particular test case is not exploitable.

Removing s-s then :)
Comment 10 Geoff Lankow (:darktrojan) 2012-06-15 22:53:56 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b589776a7912
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-06-16 06:50:16 PDT
https://hg.mozilla.org/mozilla-central/rev/b589776a7912
Comment 12 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-18 17:14:47 PDT
Does this affect 15 and need to be uplifted? If so, please go ahead and nominate for landing approval.
Comment 13 David Mandelin [:dmandelin] 2012-06-18 18:29:28 PDT
Not s-s, so no particular reason to track.

Note You need to log in before you can comment on or make changes to this bug.