Closed Bug 756581 Opened 8 years ago Closed 7 years ago

JS OOM Testing: Assertion failure: off >= 0 && (size_t) off < size, at js/src/jsopcode.cpp:786

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox17 --- wontfix
firefox18 + fixed
firefox19 + fixed
firefox20 --- fixed
firefox-esr10 --- wontfix
firefox-esr17 --- wontfix

People

(Reporter: decoder, Assigned: jimb)

References

Details

(Keywords: assertion, sec-moderate, testcase, Whiteboard: [js:inv:p1][adv-main18+][qa-])

Attachments

(2 files, 1 obsolete file)

The following command asserts on mozilla-central revision f4d51fab6cec (dbg build):

js  -e 'const libdir = "/home/decoder/Mozilla/JSBugMon/repos/ionmonkey/js/src/jit-test/lib/";' -A 8202 -f /home/decoder/Mozilla/JSBugMon/repos/ionmonkey/js/src/jit-test/tests/for-of/decompiler.js
Script had the wrong basepath in it, that's why it did not strip absolute paths, but the command should be clear anyway I guess :)

Marked this bug s-s because the assertion is indicating a read that is out-of-bounds. I don't know if you can actually do anything malicious with this.
Whiteboard: [js:inv:p1]
Can someone suggest a security rating for this and verify whether anything can be done with the out-of-bounds read?
Assignee: general → jimb
I can reproduce, with -A 8196.
Thanks, Bugzilla.
Since the decompiler does a lot of string manipulation, to keep the code simpler, its policy is for string creation functions to simply call JS_ReportOutOfMemory, but leaving things in a safe state and continue execution. Then, at certain well-defined points, we check for isExceptionPending and report the problem at that point. The assertion is due to PopStrPrec not knowing how to play along. It does seem like it could cause stray memory references.

There's only one problem with its idiosyncratic OOM policy: JS_ReportOutOfMemory doesn't set an exception. OOM is represented the same way a slow script dialog is: by returning a failure code with no exception set. So those isExceptionPending checks don't work.
That does seem to be the only use of Sprint whose return value isn't checked.
(Zero might be a safer value to return here.)
Attachment #681574 - Attachment is obsolete: true
Attachment #681573 - Flags: review?(jorendorff)
Attachment #681651 - Flags: review?(jorendorff)
jorendorff, review ping please?
Flags: needinfo?(jorendorff)
Comment on attachment 681573 [details] [diff] [review]
Correctly track out-of-memory conditions in decompiler's Sprinter type.

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

::: js/src/jsopcode.cpp
@@ +931,5 @@
> +void
> +Sprinter::reportOutOfMemory() {
> +    if (context->runtime->hadOutOfMemory)
> +        return;
> +    JS_ReportOutOfMemory(context);

Lowercase js_ since we're inside the engine.

::: js/src/jsopcode.h
@@ +481,5 @@
> +
> +    /*
> +     * Report that a string operation failed to get the memory it requested. The
> +     * first call to this function calls JS_ReportOutOfMemory, and sets this
> +     * Sprinter's outOfMemory flag; subsequent calls do nothing.

Eh, I'd either change the comment to reference runtime->hadOutOfMemory, or actually set aside a bool in class Sprinter (which has the dubious advantage that at least we'll always report *one* OOM, even if runtime->hadOutOfMemory was set for some reason unrelated to the current JSAPI call).

Just a thought.
Attachment #681573 - Flags: review?(jorendorff) → review+
Flags: needinfo?(jorendorff)
Attachment #681651 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/24fd5fefb4f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e705adb98a
Flags: in-testsuite-
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla20
Comment on attachment 681573 [details] [diff] [review]
Correctly track out-of-memory conditions in decompiler's Sprinter type.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Long-standing
User impact if declined: None; code only reachable from chrome
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): Low; code only reachable from chrome
String or UUID changes made by this patch: None.
Attachment #681573 - Flags: approval-mozilla-beta?
Attachment #681573 - Flags: approval-mozilla-aurora?
Comment on attachment 681651 [details] [diff] [review]
Correctly handle out-of-memory in JS decompiler's PopOffPrec function.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Long-standing
User impact if declined: None; code only reachable from chrome
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): Low; code only reachable from chrome
String or UUID changes made by this patch: None.
Attachment #681651 - Flags: approval-mozilla-beta?
Attachment #681651 - Flags: approval-mozilla-aurora?
Attachment #681573 - Flags: approval-mozilla-beta?
Attachment #681573 - Flags: approval-mozilla-beta+
Attachment #681573 - Flags: approval-mozilla-aurora?
Attachment #681573 - Flags: approval-mozilla-aurora+
Attachment #681651 - Flags: approval-mozilla-beta?
Attachment #681651 - Flags: approval-mozilla-beta+
Attachment #681651 - Flags: approval-mozilla-aurora?
Attachment #681651 - Flags: approval-mozilla-aurora+
Whiteboard: [js:inv:p1] → [js:inv:p1][adv-main18+]
Keywords: verifyme
Whiteboard: [js:inv:p1][adv-main18+] → [js:inv:p1][adv-main18+][qa-]
(In reply to Jim Blandy :jimb from comment #16)
> User impact if declined: None; code only reachable from chrome

If true this can't be sec-critical -- we'd need to find an example of chrome code that triggers this flaw to reach that rating. Down-rating.
Group: core-security
You need to log in before you can comment on or make changes to this bug.