Bug 11131 - Repeated string concatenation results in OOM crash
Summary: Repeated string concatenation results in OOM crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Critical
Assignee: Nobody
URL: javascript:s = "a"; while (1) { s += ...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2006-10-03 03:45 PDT by Mark Rowe (bdash)
Modified: 2007-01-26 18:50 PST (History)
2 users (show)

See Also:


Attachments
Crash log from ToT. (21.76 KB, text/plain)
2006-10-03 03:46 PDT, Mark Rowe (bdash)
no flags Details
Crash log from WebKit 418.8 (2.63 KB, text/plain)
2006-10-03 03:49 PDT, Mark Rowe (bdash)
no flags Details
Possible patch (6.47 KB, patch)
2007-01-26 06:11 PST, Andrew Wellington
mjs: review-
Details | Formatted Diff | Diff
Updated patch (6.40 KB, patch)
2007-01-26 18:00 PST, Andrew Wellington
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2006-10-03 03:45:38 PDT
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.
Comment 1 Mark Rowe (bdash) 2006-10-03 03:46:55 PDT
Created attachment 10878 [details]
Crash log from ToT.
Comment 2 Mark Rowe (bdash) 2006-10-03 03:49:42 PDT
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.
Comment 3 David Kilzer (:ddkilzer) 2006-10-03 07:25:11 PDT
(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?
Comment 4 David Kilzer (:ddkilzer) 2006-10-03 07:28:37 PDT
(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.
Comment 5 Mark Rowe (bdash) 2006-10-03 17:52:10 PDT
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 :-)
Comment 6 Mark Rowe (bdash) 2007-01-16 18:54:10 PST
<rdar://problem/4928595>
Comment 7 Andrew Wellington 2007-01-26 06:11:05 PST
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.
Comment 8 David Kilzer (:ddkilzer) 2007-01-26 08:03:27 PST
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?
Comment 9 Maciej Stachowiak 2007-01-26 14:27:19 PST
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.
Comment 10 Andrew Wellington 2007-01-26 18:00:38 PST
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.
Comment 11 Andrew Wellington 2007-01-26 18:02:54 PST
(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.
Comment 12 Maciej Stachowiak 2007-01-26 18:37:35 PST
Comment on attachment 12701 [details]
Updated patch

r=me
Comment 13 Alice Liu 2007-01-26 18:50:43 PST
landed as r19178