[webkit-dev] malloc(0)

[Myles C. Maxfield]

Thanks for the quick responses and the bug link about fastMalloc(0).

In comment 11 <https://bugs.webkit.org/show_bug.cgi?id=55097#c11>, Eric
Seidel says that he was going to start a discussion on webkit-dev, but I
haven't been able to find such a conversation (by looking both on gmane and
on google). All I've been able to find is this
thread<https://lists.webkit.org/pipermail/webkit-dev/2011-June/016967.html>,
which is only a single post. Does anyone know where this conversation takes
place?

That being said, Darin Adler is right; the original patch was intended to
improve performance, not because something was breaking. It seems like most
of the conversation regarding that patch is about any possible performance
increase. In addition, Eric Seidel's patch was to guarantee that no one
tries to call fastMalloc with a size of 0; I'm merely concerned about the
value of the pointer returned when someone does call that.

I think the fact that ICU thinks that a null pointer is an invalid string
isn't necessarily an incorrect one. It's just a quirk of an interface to a
library. I don't think that a good solution is to change the interface of
ICU and try to upstream a patch to ICU - I think a better solution would be
to work around this "requirement" of ICU inside WebKit.

I see two different solutions:
1) Document the fact that, in order to use webkit, you need an
implementation of malloc(0) that doesn't return null pointers. This way,
the burden of solving this problem is pushed downstream.
2) Find some place along the every pipeline from malloc(0) to ICU, and
arbitrarily modify the pointer to a non-zero value. This is probably the
best solution, but a real fix of this nature requires finding all the
places where pointers can be passed into ICU.

If I solve (via option 2) just my one particular occurrence of this
problem, I see three different places to arbitrarily modify the pointer
given to ICU:
1) change the m_copyData16 pointer in StringImpl to an arbitrary value, and
check for that arbitrary value on destruction
2) change the characters() accessor to check if m_copyData16 is null, and
return an arbitrary value if it is. This works because callers of
characters() shouldn't ever delete the pointer nor dereference the pointer
(since the string's length is 0)
3) check for null at the call site of the ICU function.

I (perhaps prematurely) uploaded a CL
implementing<https://bugs.webkit.org/show_bug.cgi?id=88936>option 2.
What do you think?

--Myles

On Wed, Jun 13, 2012 at 2:09 AM, Zoltan Horvath <zoltan at webkit.org> wrote:
>
> Hi,
>
> The bug report about fastMalloc(0):
> https://bugs.webkit.org/show_bug.cgi?id=55097
>
> Brewmp had conditions for fastMalloc(0) earlier, but it was removed in:
>
http://trac.webkit.org/changeset/95555/trunk/Source/JavaScriptCore/wtf/FastMalloc.cpp
>
> Cheers,
> <Zoltan>
>
>
> On Wed, 13 Jun 2012 00:08:48 +0200, Adam Barth <abarth at webkit.org> wrote:
>
>> There was some discussion about how to handle malloc(0) a year or so
>> ago.  I don't remember if it was on webkit-dev, but you might want to
>> check the archives.  Eric Seidel might remember what conclusions (if
>> any) we came to.
>>
>> Adam
>>
>>
>> On Tue, Jun 12, 2012 at 3:03 PM, Myles C. Maxfield
>> <myles.maxfield at gmail.com> wrote:
>>>
>>> Hello,
>>> I'm compiling WebKit with a malloc() implementation that returns NULL
>>> for malloc(0). According to C99, this is valid: "If the size of the
>>> space requested is zero, the behavior is implementation- defined:
>>> either a null pointer is returned, or the behavior is as if the size
>>> were some nonzero value, except that the returned pointer shall not be
>>> used to access an object."
>>>
>>> I noticed that this caused a problem in one particular place
>>> (WTF::StringImpl::getData16SlowCase()) where the code allocates
>>> (constant * length) bytes for an (empty) string, and provides an
>>> accessor that exposes this pointer. This pointer was being passed to
>>> ICU, which didn't perform the requested function because it looked
>>> like one of the arguments was invalid, even though it was just empty.
>>>
>>> I have worked around this one particular occurrence in my local
>>> version of WebKit fork, but I'm wondering how often this pattern
>>> occurs. Is my fix worth upstreaming?  Is it worth trying to find,
>>> "fix," and upstream every occurrence of this pattern? Or is this
>>> particular behavior of malloc() an unstated requirement of building
>>> WebKit? If the latter is true, perhaps it's worth explicitly stating
>>> this somewhere? What is the opinion of the community?
>>>
>>> Thanks,
>>> Myles
>>> _______________________________________________
>>> webkit-dev mailing list
>>> webkit-dev at lists.webkit.org
>>> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>>
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev at lists.webkit.org
>> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120613/ef0b1551/attachment.html>