Bug 34569 - Don't call CRASH() in fastMalloc and fastCalloc when the requested memory size is 0
Summary: Don't call CRASH() in fastMalloc and fastCalloc when the requested memory siz...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 33564
  Show dependency treegraph
 
Reported: 2010-02-04 03:15 PST by Kwang Yul Seo
Modified: 2010-02-11 19:29 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.19 KB, patch)
2010-02-04 03:18 PST, Kwang Yul Seo
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff
Patch (2.16 KB, patch)
2010-02-04 19:53 PST, Kwang Yul Seo
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff
Patch (2.38 KB, patch)
2010-02-05 00:35 PST, Kwang Yul Seo
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff
Patch (2.37 KB, patch)
2010-02-05 02:19 PST, Kwang Yul Seo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2010-02-04 03:15:22 PST
With USE_SYSTEM_MALLOC=1, fastMalloc, fastCalloc and fastRealloc call CRASH() if the return value of malloc, calloc and realloc is 0. However, those functions can return 0 when the request size is 0.

From the libc manual of malloc,

"If size is 0, then malloc() returns either NULL, or a unique pointer value that can later be successfully passed to free()."

Though it seems malloc returns a unique pointer in most systems, 0 can be returned in some systems. For instance, BREW's MALLOC returns 0 when size is 0.

Add a check if the request size is 0 or not before calling CRASH(). As the condition is short-circuited, it adds no runtime overhead in usual allocation scenario.
Comment 1 Kwang Yul Seo 2010-02-04 03:18:49 PST
Created attachment 48126 [details]
Patch
Comment 2 Zoltan Horvath 2010-02-04 04:25:42 PST
The patch looks okay to me.
Comment 3 Darin Adler 2010-02-04 11:16:14 PST
Comment on attachment 48126 [details]
Patch

I suspect higher level code depends on not getting 0 when a size of 0 is passed. A better fix may be to change the size from 0 to 1 on the BREW system.
Comment 4 Alexey Proskuryakov 2010-02-04 12:38:15 PST
Comment on attachment 48126 [details]
Patch

I agree with Darin - even when using system malloc, FastMalloc should provide standardized behavior on all platforms.

Please also see bug 29026 which covers a related issue.
Comment 5 Kwang Yul Seo 2010-02-04 17:27:27 PST
(In reply to comment #3)
> (From update of attachment 48126 [details])
> I suspect higher level code depends on not getting 0 when a size of 0 is
> passed. A better fix may be to change the size from 0 to 1 on the BREW system.

I agree, but I don't want to introduce size check on the BREW system because it adds additional overhead to every allocation. 

This isn't a BREW specific issue because malloc can return 0 in other systems too.

Why don't we simply retry when malloc(0) returns 0?

void* fastMalloc(size_t n)
{
    ASSERT(!isForbidden());

#if ENABLE(FAST_MALLOC_MATCH_VALIDATION)
    TryMallocReturnValue returnValue = tryFastMalloc(n);
    void* result;
    returnValue.getValue(result);
#else
    void* result = malloc(n);
#endif

    if (!result)
        // malloc(0) can return 0.
        // To make sure that fastMalloc never returns 0, increase the size
        // to 1 and try again. This introduces no additional overhead
        // in usual allocations.
        if (n)
            CRASH();
        else
            return fastMalloc(1);

    return result;
}

It adds no overhead because non zero size request never returns 0 unless allocation fails.

However, I am not sure what to do with realloc because realloc(p, 0) is equivalent to free(p). This is the exact duplicate of  https://bugs.webkit.org/show_bug.cgi?id=29026
Comment 6 Kwang Yul Seo 2010-02-04 19:53:58 PST
Created attachment 48192 [details]
Patch

If malloc(0) returns NULL in fastMalloc, increase the size to 1 and retry. Do the same for fastCalloc.

Leave out fastRealloc as it seems there is no code which calls fastRealloc(p, 0) currently. This needs to be discussed further in https://bugs.webkit.org/show_bug.cgi?id=29026
Comment 7 Alexey Proskuryakov 2010-02-04 23:56:15 PST
Comment on attachment 48192 [details]
Patch

I don't see how checking after calling malloc is better than checking before calling it. Normal case just gets one more branch either way, but fastMalloc(0) is slower if you check after malloc returns 0.

We don't need to add the branch on platforms that don't return 0, please protect the new code with preprocessor checks. Any future port that runs into this can add themselves to the check.

>         (WTF::fastRealloc):

You still had this in ChangeLog.
Comment 8 Kwang Yul Seo 2010-02-05 00:17:53 PST
(In reply to comment #7)
> (From update of attachment 48192 [details])
> I don't see how checking after calling malloc is better than checking before
> calling it. Normal case just gets one more branch either way, but fastMalloc(0)
> is slower if you check after malloc returns 0.
> 

Checking after malloc is actually better than checking before calling it. fastMalloc(n!=0) returns non-NULL unless allocation fails, so "if (!result)" branch is not taken at all in normal case. 

malloc(0) or allocation failure take this branch and an additional check is performed. Because both are not frequent, I think this is okay.

> We don't need to add the branch on platforms that don't return 0, please
> protect the new code with preprocessor checks. Any future port that runs into
> this can add themselves to the check.

I agree. I will add guards. 

> 
> >         (WTF::fastRealloc):
> 
> You still had this in ChangeLog.

Ooops. Sorry for the mistake.
Comment 9 Kwang Yul Seo 2010-02-05 00:35:09 PST
Created attachment 48210 [details]
Patch

Put platform guards.
Comment 10 Alexey Proskuryakov 2010-02-05 01:30:43 PST
Comment on attachment 48210 [details]
Patch

I now see what you're saying - there's already a branch for null result, so you are not adding one.

+        // malloc(0) can return 0 in some systems.
+        // To make sure that fastMalloc never returns 0,
+        // retry with fastMalloc(1).

Saying "in some systems" can be confusing. I'd say "Behavior of malloc(0) is implementation defined". Also, these lines are extremely short, there is no need to wrap comments like this.

r=me. Someone can fix these when landing, or you could submit a corrected patch for commit queue.
Comment 11 Kwang Yul Seo 2010-02-05 02:19:37 PST
Created attachment 48216 [details]
Patch

Change the comment as Alexey suggested. Thank you for reviewing!
Comment 12 Zoltan Horvath 2010-02-05 04:29:31 PST
Committed revision 54419.
http://trac.webkit.org/changeset/54419