705855 - NSLOTS_LIMIT is too high for JIT'd code

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

While reviewing bug 701093, I noticed that the NSLOTS_LIMIT of 2^29 is probably too high. In JM and IM we emit code like:

    mov dest, [slots + index * 8]

The |index * 8| is signed, so to not overflow the index must be < 2^28. With -m -n:

js> Q = []
[]
js> i = 0;
0
js> while (true) {
Q[i] = i;
i++;
}
typein:4: out of memory
js> Q.length
498991104
js> function f() { var a; for (var i = 0; i < 10000; i++) { a = Q[498991103]; } return a; }
js> f();
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x00000000efaffff8
0x0000000100ba2fd5 in ?? ()

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Comment 2

[David Mandelin [:dmandelin]]

Comment on attachment 578782 [details] [diff] [review]
fix

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

::: js/src/jsobj.cpp
@@ +115,5 @@
>  using namespace js;
>  using namespace js::gc;
>  using namespace js::types;
>  
> +JS_STATIC_ASSERT(int32((JSObject::NSLOTS_LIMIT - 1) * sizeof(Value)) > 0);

This might want to be something like 

JS_STATIC_ASSERT(int32((JSObject::NSLOTS_LIMIT - 1) * sizeof(Value)) == int64((JSObject::NSLOTS_LIMIT - 1) * sizeof(Value)) );

but I don't think it's that important.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bafbaa9ac33

Comment 4

[Ian Melven :imelven]

from ed morley : 

https://hg.mozilla.org/mozilla-central/rev/6bafbaa9ac33

marking target milestone mozilla11 and RESOLVED FIXED

Comment 5

[Johnny Stenback (:jst)]

dvander, is this good to take for beta? If so, please request approval.

Comment 6

[Daniel Veditz [:dveditz]]

Would we need a different patch for beta or can we simply approve and land this one?

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage comment]

This bug is being tracked for landing on ESR branch.  Please land patches on http://hg.mozilla.org/releases/mozilla-esr10/by Thursday March 1, 2012 in order to be ready for go-to-build on Friday March 2, 2012.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more information.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

David, is landing this on your radar for today or tomorrow?  We need this fix in before Thursday to make sure we're ready to go to build. Let me know if there's any concern about it landing cleanly or anything we can do to help.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/releases/mozilla-esr10/rev/c9d8fd7a6964

Comment 10

[Jason Smith [:jsmith]]

Verified fixed in Firefox 10.0.3esr. Ran that while loop for a few minutes and did not see it error out.

Comment 11

[Jason Smith [:jsmith]]

Verified fixed in Firefox 11.

Comment 12

[Jason Smith [:jsmith]]

Re-tested this on FF 10.0.3esr to wait for the while loop to finish. After the out of memory error is reported, nothing ends up being written to the array that was being iterated over. Should this be reopened to avoid a possible regression?

Comment 13

[Jason Smith [:jsmith]]

Verified fixed in FF 12. In comparing the behavior on FF 11 & 12 to FF 10 ESR, I think there may be a regression in the fix for FF 10 ESR, as the behavior is significantly different and does not make sense. Requesting to reopen bug for FF 10 ESR.

Comment 14

[juan becerra [:juanb]]

David, is comment #12 of concern and should this bug be re-opened?

Comment 15

[u279076]

I was speaking to Jason about this earlier and I think it's okay to mark this verified for the time being. Correct me if I am wrong, but the exploit condition was the fact that kernel error was being thrown. In Firefox 11, 12, and ESR, this is no longer the case.

The difference now being that Q.length returns a value in Firefox 11 and 12, but does not in Firefox 10.0.3esr. I'm thinking this might be a different bug, and not necessarily a security issue.

Please reopen if I am wrong.

Comment 16

[Jason Smith [:jsmith]]

Opened bug 733531 to track the Q.length issue with ESR.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Verified fixed in trunk with 4/12 nightly jsshell. No kernel error now.