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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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]

Attachment: add integer properties to rest argument

Comment 2

[Brian Hackett [Laid off!]]

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]

Attachment: add null check

Comment 4

[Daniel Holbert [:dholbert]]

(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]]

(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]

This bug is not a security bug. On release builds, the test passes successfully.

Comment 7

[Christian Holler (:decoder)]

(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]

This particular test case is not exploitable.

Comment 9

[Christian Holler (:decoder)]

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

Removing s-s then :)

Comment 10

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b589776a7912

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b589776a7912

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Does this affect 15 and need to be uplifted? If so, please go ahead and nominate for landing approval.

Comment 13

[David Mandelin [:dmandelin]]

Not s-s, so no particular reason to track.