Bug 11131 – Repeated string concatenation results in OOM crash

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Rowe (bdash)]

This bug was noted while looking at the test case attached to https://bugzilla.mozilla.org/show_bug.cgi?id=355069.  This crashes ToT WebKit almost instantly.

[Mark Rowe (bdash)]

Created attachment 10878 [details]
Crash log from ToT.

[Mark Rowe (bdash)]

Created attachment 10879 [details]
Crash log from WebKit 418.8

A similar problem occurs with released WebKit.  A failed malloc results in abort being called, and Safari disappears without a crash log.  This log is retrieved from attached GDB.

[David Kilzer (:ddkilzer)]

(In reply to comment #0)
> This bug was noted while looking at the test case attached to
> https://bugzilla.mozilla.org/show_bug.cgi?id=355069.  This crashes ToT WebKit
> almost instantly.

How will this JavaScript code NOT eventually cause an out-of-memory error?  The fact that it causes an OOM almost immediately in ToT means that the JavaScript is running very fast, which is good right?!  :)

What I'm asking is what should the JavaScript engine do to prevent an OOM in this case since the code will always end up with an OOM the way it's written?

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> What I'm asking is what should the JavaScript engine do to prevent an OOM in
> this case since the code will always end up with an OOM the way it's written?

Firefox 1.5.0.7 looks like it may throw an OOM exception (or it raises some kind of error condition) when the JavaScript engine runs out of memory, but it doesn't crash the browser.  This is probably the desired behavior we want.

[Mark Rowe (bdash)]

David, it is expected that it will cause an out of memory error, but not that it will segfault or abort as a result of it.  JavaScript code crashing the host application is never a good thing :-)

[Mark Rowe (bdash)]

<rdar://problem/4928595>

[Andrew Wellington]

Created attachment 12680 [details]
Possible patch

OK, I've never done anything in JavaScriptCore before so this may be totally wrong... but still it does fix the issue :-)

It's relatively simple: in UString::UString(const UString &a, const UString &b) we check for the out-of-memory case in the allocation which returns NULL and then return a null UString instead of blindly trying a memcpy which will crash. Then KJS::add checks if it receives a null UString from its add operation, and if so throws an exception, otherwise we continue on as we did before.

[David Kilzer (:ddkilzer)]

Comment on attachment 12680 [details]
Possible patch

NOTE: I am NOT a reviewer...more of a random patch commentator.

>+        } else {
>+            return jsString(value);
>+        }

Unneeded curly braces here and in a few other places.

>+shouldThrow('s = "a"; while (1) { s += s; }', '"Error: Out of memory"');

How "fast" does this actually run, e.g., how long does this test take to run?  (Hmm...I suppose it would depend on how much memory the system has that it runs on.)  Perhaps a string longer than one character should be used, though, so it will run faster.  Maybe 512 or 1024 characters?

[Maciej Stachowiak]

Comment on attachment 12680 [details]
Possible patch

Looks good but please fix style issues (extra braces). I'll r+ once you upload a fixed patch.

[Andrew Wellington]

Created attachment 12701 [details]
Updated patch

Updated patch to remove extra braces, and as an added bonus made it more efficient -- no need to check a.data() && b.data(), just the expanded one.

[Andrew Wellington]

(In reply to comment #8)
> How "fast" does this actually run, e.g., how long does this test take to run? 
> (Hmm...I suppose it would depend on how much memory the system has that it runs
> on.)  Perhaps a string longer than one character should be used, though, so it
> will run faster.  Maybe 512 or 1024 characters?
> 

This is an exponential test, we're doubling the length each time. It takes less than a second on my PowerMac G5 1.8GHz single processor, 2.5GB RAM.

[Maciej Stachowiak]

Comment on attachment 12701 [details]
Updated patch

r=me

[Alice Liu]

landed as r19178