Closed Bug 705855 Opened 8 years ago Closed 8 years ago

NSLOTS_LIMIT is too high for JIT'd code

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

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

People

(Reporter: dvander, Assigned: dvander)

Details

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

Attachments

(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;
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 ?? ()
Whiteboard: [sg:critical]
Attached patch fixSplinter Review
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #578782 - Flags: review?(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.
Attachment #578782 - Flags: review?(dmandelin) → review+
from ed morley : 

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

marking target milestone mozilla11 and RESOLVED FIXED
Status: ASSIGNED → RESOLVED
Closed: 8 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 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.
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.