Summary: | Repeated string concatenation results in OOM crash | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Rowe (bdash) <mrowe> | ||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Critical | CC: | ddkilzer, sanjay12 | ||||||||||
Priority: | P1 | Keywords: | InRadar | ||||||||||
Version: | 420+ | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
URL: | javascript:s = "a"; while (1) { s += s; } | ||||||||||||
Attachments: |
|
Description
Mark Rowe (bdash)
2006-10-03 03:45:38 PDT
Created attachment 10878 [details]
Crash log from ToT.
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.
(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? (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. 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 :-) 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 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 on attachment 12680 [details]
Possible patch
Looks good but please fix style issues (extra braces). I'll r+ once you upload a fixed patch.
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.
(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 on attachment 12701 [details]
Updated patch
r=me
landed as r19178 |