Bug 29026 – CRASH: fastRealloc crashes on realloc(ptr, 0)

• Component: Web Template Framework
• Status: REOPENED

[Mike Belshe]

The standard for realloc(ptr, 0) is that it should return NULL and free the ptr.

In our implementation, this will crash.

This was discovered because I found a case in WebKit which attempts to realloc(ptr, 0):
WTF::fastRealloc+0x10
WebCore::HTMLTokenizer::enlargeScriptBuffer+0x41
WebCore::HTMLTokenizer::parseComment+0x2a
WebCore::HTMLTokenizer::parseTag+0x1141
WebCore::HTMLTokenizer::write+0x414
WebCore::FrameLoader::write+0x36b
WebCore::FrameLoader::addData+0x12

To get here, we have to read data input off the socket which contains a partial page ending with "<!--".  It's a little hard to reproduce.

I believe the fixed code should look something like this (simply adding a check for n > 0 before calling CRASH):


void* fastRealloc(void* p, size_t n)
{
    ASSERT(!isForbidden());

#if ENABLE(FAST_MALLOC_MATCH_VALIDATION)
    TryMallocReturnValue returnValue = tryFastRealloc(p, n);
    void* result;
    returnValue.getValue(result);
#else
    void* result = realloc(p, n);
#endif

    // Crash if the result is NULL and the size requested was greater
    // than zero.  realloc(p, 0) returns NULL intentionally.
    if (!result  && n > 0)
        CRASH();
    return result;
}

[Mark Rowe (bdash)]

This is the intentional behavior of fastRealloc.  If a caller can handle a null return it should explicitly call the tryFastRealloc version.  Callers that don't call the "try" version are assumed to not handle a null return and so we abort if we encounter that case.

[Mark Rowe (bdash)]

Closing as WONTFIX since this is by design.  You should file a new bug about updating HTMLTokenizer::enlargeScriptBuffer to handle this correctly in the case that you refer to.

[Mike Belshe]

I'm going to reopen just for a bit so we can get further debate on this - switching to tryFastRealloc() would work.

But semantically, I'm not sure if that is the best?

If you look at FastMalloc.h, about fastRealloc(), it says, "// These functions call CRASH() if an allocation fails."

This wasn't an allocation failure.  realloc(), by specification, returns NULL in two cases - either on allocation failure, or when allocating to size zero.

If we take your suggestion, the fastRealloc() is behaving slightly differently from the realloc standard.

Two choices:
a) fastRealloc should continue to crash on this case.  If so, we should document that it diverges from the standard, and crashes even in this case (which is not an allocation failure).
b) fastRealloc should not crash in this case.

I did a quick search through the code, and I don't see much usage of tryFastRealloc() at all.  This leads me to believe that for this case, maybe fastRrealloc() should be changed?

Let me know how you guys would like to proceed.  At the very least, I'd like to update the comments on fastRealloc() to reflect our decision on this API.

[Dimitri Glazkov (Google)]

I think Mike is right that fastRealloc shouldn't crash if it's not failing allocation -- especially when allocating to 0, in which case NULL is the valid output.

[Mark Rowe (bdash)]

The entire reason that the "try" version of the functions exist is as a way for the caller to say "I'm ok with getting 0 back from this".  It makes it clear that the author has considered this case and that the code accommodates this.  The same cannot be said about the uses of "fastRealloc" in general.

[Mike Belshe]

(In reply to comment #5)
> The entire reason that the "try" version of the functions exist is as a way for
> the caller to say "I'm ok with getting 0 back from this".  It makes it clear
> that the author has considered this case and that the code accommodates this. 
> The same cannot be said about the uses of "fastRealloc" in general.

That's not what the comments say.  The comment in fastmalloc says, "These functions call CRASH() if an allocation fails."

Thats a subtle, but different meaning from what you suggest because in this case, the allocation did not fail.  We can change it, I guess.

[Alexey Proskuryakov]

I agree that fastRealloc should have the same semantics as realloc, or be renamed. However, I'm not sure which specification you're citing, as the behavior is different per Mac OS X man page:

"If ptr is NULL, realloc() is identical to a call to malloc() for size bytes. If size is zero and ptr is not NULL, a new, minimum sized object is allocated and the original object is freed."

As a result, the behavior of realloc(0, 0) is implementation defined, but realloc(nonNull, 0) doesn't return 0 unless minimum sized object allocation fails.

[Mike Belshe]

(In reply to comment #7)
> I agree that fastRealloc should have the same semantics as realloc, or be
> renamed. However, I'm not sure which specification you're citing, as the
> behavior is different per Mac OS X man page:
> 
> "If ptr is NULL, realloc() is identical to a call to malloc() for size bytes.
> If size is zero and ptr is not NULL, a new, minimum sized object is allocated
> and the original object is freed."
> 
> As a result, the behavior of realloc(0, 0) is implementation defined, but
> realloc(nonNull, 0) doesn't return 0 unless minimum sized object allocation
> fails.

Good digging, Alexey! I was using this reference:  http://www.opengroup.org/onlinepubs/009695399/functions/realloc.html

So it appears that BSD (and Mac OSX) already diverges from the C spec.

In light of this, the bug gets deeper!  fastRealloc() is implemented using realloc(), and realloc on different platforms behaves differently in this case.  We should document that, and normalize the fastRealloc() to always behave like BSD realloc (I assume that would be apple's preferred API?)

[Mark Rowe (bdash)]

(In reply to comment #8)
> (In reply to comment #7)
> > I agree that fastRealloc should have the same semantics as realloc, or be
> > renamed. However, I'm not sure which specification you're citing, as the
> > behavior is different per Mac OS X man page:
> > 
> > "If ptr is NULL, realloc() is identical to a call to malloc() for size bytes.
> > If size is zero and ptr is not NULL, a new, minimum sized object is allocated
> > and the original object is freed."
> > 
> > As a result, the behavior of realloc(0, 0) is implementation defined, but
> > realloc(nonNull, 0) doesn't return 0 unless minimum sized object allocation
> > fails.
> 
> Good digging, Alexey! I was using this reference: 
> http://www.opengroup.org/onlinepubs/009695399/functions/realloc.html
> 
> So it appears that BSD (and Mac OSX) already diverges from the C spec.

Quoting from <http://www.opengroup.org/onlinepubs/009695399/functions/realloc.html>:
If size is 0, either a null pointer or a unique pointer that can be successfully passed to free() shall be returned.

Mac OS X's malloc returns a unique pointer that can be successfully passed to free.  That's completely in line with the specification.

[Mike Belshe]

Heh - I guess I quoted the spec which allows both.  Several exist.
http://www.cplusplus.com/reference/clibrary/cstdlib/realloc/
http://msdn.microsoft.com/en-us/library/xbebcx7d(VS.71).aspx

And of course, the tcmalloc implementation currently used in WTF does this too (see FastMalloc.cpp)

So - my point remains that we now have different ports using different allocators that may have different return values for this case.

If you use the USE_SYSTEM_MALLOC in Webkit on mac, you'll get the mac version of realloc() which won't crash in fastRealloc() in this case.  On the other hand, if you don't set that env var, you'll use tcmalloc, which will :-)

Seems inconsistent, no?

[Alexey Proskuryakov]

(In reply to comment #10)
> Heh - I guess I quoted the spec which allows both.  Several exist.

The spec to quote would be C99, not vendor documentation (as the behavior is implementation defined, vendors must document what they do, not repeat the spec).

Anyway, it seems clear that standardizing on a variant of BSD or MSVC behavior would be good (I believe that the latter makes another step forward by returning a non-zero pointer to a newly allocated memory block that can't be referenced).

[Dimitri Glazkov (Google)]

While you guys are finalizing this discussion, bug 29313 (and patch to fix) filed for the crash.

[Alexey Proskuryakov]

When this is fixed, changes made in bug 29313 should be reverted.