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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

Comment 1

[Christian Holler (:decoder)]

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.

Comment 2

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

Can someone suggest a security rating for this and verify whether anything can be done with the out-of-bounds read?

Comment 3

[Jim Blandy :jimb]

I can reproduce, with -A 8196.

Comment 4

[Jim Blandy :jimb]

Thanks, Bugzilla.

Comment 5

[Jim Blandy :jimb]

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.

Comment 6

[Jim Blandy :jimb]

Attachment: Correctly track out-of-memory conditions in decompiler's Sprinter type.

Comment 7

[Jim Blandy :jimb]

Attachment: Correctly handle out-of-memory in JS decompiler's PopOffPrec function.

Comment 8

[Jim Blandy :jimb]

That does seem to be the only use of Sprint whose return value isn't checked.

Comment 9

[Jim Blandy :jimb]

Attachment: Correctly handle out-of-memory in JS decompiler's PopOffPrec function.

(Zero might be a safer value to return here.)

Comment 10

[Jim Blandy :jimb]

Try run looks good: https://tbpl.mozilla.org/?tree=Try&rev=22eea6ba78d8

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

jorendorff, review ping please?

Comment 12

[Jason Orendorff [: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.

Comment 13

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24fd5fefb4f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e705adb98a

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/24fd5fefb4f2
https://hg.mozilla.org/mozilla-central/rev/b6e705adb98a

Comment 15

[Jim Blandy :jimb]

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.

Comment 16

[Jim Blandy :jimb]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e1a5e0a809ff
https://hg.mozilla.org/releases/mozilla-aurora/rev/f78e447e52f6

https://hg.mozilla.org/releases/mozilla-beta/rev/38e0dbf44ee5
https://hg.mozilla.org/releases/mozilla-beta/rev/8482faeb8a8b

Comment 18

[Daniel Veditz [:dveditz]]

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