Bug 18670 - fastMalloc should crash on failed allocs
Summary: fastMalloc should crash on failed allocs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-21 14:26 PDT by Eric Seidel (no email)
Modified: 2008-08-13 06:08 PDT (History)
7 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-04-21 14:26:57 PDT
fastMalloc should crash on failed allocs

We don't check the results of of malloc most of the time anyway.  We might as well make fastMalloc rash when an alloc fails.  Yes, we'd have to fix the few cases where we check for null, to instead do a range-check before passing it off to malloc.
Comment 1 Darin Adler 2008-04-21 15:35:01 PDT
I agree completely that for callers that can't handle null it would be good to do something like abort() or CRASH().

Some callers do want to handle null; I'm not sure if there are any that are actually doing it. RenderTableSection::ensureRows is an example: it wants to return false if it can't grow the grid. But instead it should be designed so we don't need to allocate something based on the total number of rows.

Bug 9806 is one thing to look at. Another is <http://trac.webkit.org/projects/webkit/changeset/10959>.
Comment 2 Mark Rowe (bdash) 2008-04-21 16:27:57 PDT
KJS::UString is a good example of code that already checks for failure of fastMalloc and fastRealloc.
Comment 3 Eric Seidel (no email) 2008-04-21 16:57:32 PDT
Sure, but all of the above mentioned cases could be replaced by a pre-alloc check against some fixed size.  One could even provide a "safeMalloc", "mallocWhichMayFail" which did what the current fastMalloc does...  Every other time we assume that fastMalloc gives back a non-NULL pointer, or we crash.
Comment 4 Mark Rowe (bdash) 2008-04-21 17:59:42 PDT
How can it be replaced by a check against a fixed size?  Allocating a very large number of a smaller strings can trigger a situation that will lead to fastMalloc failing just as easily as allocating a small number of very large strings.
Comment 5 Eric Seidel (no email) 2008-04-23 18:17:46 PDT
(In reply to comment #4)
> How can it be replaced by a check against a fixed size?  Allocating a very
> large number of a smaller strings can trigger a situation that will lead to
> fastMalloc failing just as easily as allocating a small number of very large
> strings.

Sure, and that fastMalloc should fail (like it would today), and CRASH right then and there (like it should, but doesn't today).  The only "valid" fastMalloc failure conditions, are when fastMalloc is used as a integer size check for large user-provided data.  Those can be replaced with a check against some arbitrary fixed size.  ("WebCore does not support allocating more than 2G of data", or more likely a much smaller number.)

You end up with two (logical) allocators. One which does small allocations, and never fails (when it does, you CRASH then and there.)  A second, which could be used for larger allocations, but that you have to null check (what we have today).

Hum... I guess another way to look at this is to just turn fastMalloc into a wrapper function around a new fastMallocInternal() brave clients who are sure they will NULL check, can use the internal one.  Less brave clients get the wrapper by default, which CRASHs on NULL.

Comment 6 Mark Rowe (bdash) 2008-04-23 19:17:21 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > How can it be replaced by a check against a fixed size?  Allocating a very
> > large number of a smaller strings can trigger a situation that will lead to
> > fastMalloc failing just as easily as allocating a small number of very large
> > strings.
> 
> Sure, and that fastMalloc should fail (like it would today), and CRASH right
> then and there (like it should, but doesn't today). 

JavaScriptCore detects this case and propagates it to the calling script as an "Out of memory" exception.  How is crashing an improvement?
Comment 7 Alexey Proskuryakov 2008-08-13 00:13:20 PDT
Fixed by Mitz in <http://trac.webkit.org/projects/webkit/changeset/35691>.
Comment 8 David Kilzer (:ddkilzer) 2008-08-13 06:08:41 PDT
See <rdar://problem/6121636>.