Bug 34569 – Don't call CRASH() in fastMalloc and fastCalloc when the requested memory size is 0

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Kwang Yul Seo]

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.

[Kwang Yul Seo]

Created attachment 48126 [details]
Patch

[Zoltan Horvath]

The patch looks okay to me.

[Darin Adler]

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.

[Alexey Proskuryakov]

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.

[Kwang Yul Seo]

(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

[Kwang Yul Seo]

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

[Alexey Proskuryakov]

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.

[Kwang Yul Seo]

(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.

[Kwang Yul Seo]

Created attachment 48210 [details]
Patch

Put platform guards.

[Alexey Proskuryakov]

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.

[Kwang Yul Seo]

Created attachment 48216 [details]
Patch

Change the comment as Alexey suggested. Thank you for reviewing!

[Zoltan Horvath]

Committed revision 54419.
http://trac.webkit.org/changeset/54419