Closed Bug 705855 Opened 12 years ago Closed 12 years ago

NSLOTS_LIMIT is too high for JIT'd code


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox8 - wontfix
firefox9 - wontfix
firefox10 - affected
firefox11 + verified
firefox12 + verified
firefox-esr10 11+ verified
status1.9.2 --- unaffected


(Reporter: dvander, Assigned: dvander)


(Whiteboard: [sg:critical][qa!])


(1 file)

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;
js> while (true) {
Q[i] = i;
typein:4: out of memory
js> Q.length
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 ?? ()
Whiteboard: [sg:critical]
Attached patch fixSplinter Review
Assignee: general → dvander
Attachment #578782 - Flags: review?(dmandelin)
Comment on attachment 578782 [details] [diff] [review]

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.
Attachment #578782 - Flags: review?(dmandelin) → review+
from ed morley :

marking target milestone mozilla11 and RESOLVED FIXED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
dvander, is this good to take for beta? If so, please request approval.
Would we need a different patch for beta or can we simply approve and land this one?
Whiteboard: [sg:critical] → [sg:critical][qa+]
[Triage comment]

This bug is being tracked for landing on ESR branch.  Please land patches on Thursday March 1, 2012 in order to be ready for go-to-build on Friday March 2, 2012.

See for more information.
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.
Attachment #578782 - Flags: approval-mozilla-esr10?
Attachment #578782 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Verified fixed in Firefox 10.0.3esr. Ran that while loop for a few minutes and did not see it error out.
Verified fixed in Firefox 11.
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?
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.
David, is comment #12 of concern and should this bug be re-opened?
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.
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Opened bug 733531 to track the Q.length issue with ESR.
Group: core-security
Verified fixed in trunk with 4/12 nightly jsshell. No kernel error now.
You need to log in before you can comment on or make changes to this bug.