Bug 176310 – Remove "malloc" and "free" use

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Remove "malloc" and "free" use

[Yusuke Suzuki]

Created attachment 319783 [details]
Patch

[Saam Barati]

Comment on attachment 319783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319783&action=review

> Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:57
> +    LOGFONT logFont { 0 };

There is code below that takes the address of this variable. Is that safe?

[Darin Adler]

Comment on attachment 319783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319783&action=review

Great general direction to get rid of malloc/free.

But I don’t think we need MallocPtr at any of these call sites. And I am wondering why we have it at all. See comment below.

> Source/JavaScriptCore/API/JSWrapperMap.mm:79
> +    auto buffer = MallocPtr<char>::malloc(header + strlen(firstColon + 1) + 1);

I would not have used MallocPtr<char>::malloc(size) here.

- I would have suggested Vector<char, SOME_CONSTANT> here instead. Pluses: 1) Avoids heap allocation entirely for common case. 2) Familiar idiom used many places in WebKit. Minuses: 1) Uses more stack space than malloc. 2) Extra overhead to keep track of size and capacity for resizing, which is wasteful in cases where we do not resize. 3) Doesn’t build in type casting for in cases where we need to allocate a certain number of bytes and cast to a pointer type other than "char*". 4) May involve unwanted zeroing of bytes in some cases.

- My second choice would have been std::make_unique<char[]>(size). Pluses: 1) Built into the standard library. Minuses: 1) Zeroes out the bytes.

I am not sure why WebKit needs the MallocPtr class unless there is are places we definitely can’t use either of these. I did not check where we are already using it to see why we didn’t use either of my alternatives here.

> Source/WTF/wtf/Assertions.cpp:107
> +        auto buffer = MallocPtr<char>::malloc(length + 1);

Vector<char, SOME_CONSTANT>?

> Source/WTF/wtf/Assertions.cpp:114
>          CFRelease(str);
>          CFRelease(cfFormat);

If we are already touching this code to deal with object lifetimes, we should consider changing these to RetainPtr.

> Source/WTF/wtf/Assertions.cpp:135
> +            auto buffer = MallocPtr<char>::malloc(size);

Vector<char, SOME_CONSTANT>?

The way this code silently stops and logs nothing if the allocation fails is something we do not need to preserve.

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:141
> +    auto linkedFonts = MallocPtr<WCHAR>::malloc(linkedFontsBufferSize);

Vector<WCHAR, SOME_CONSTANT>?

>> Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:57
>> +    LOGFONT logFont { 0 };
> 
> There is code below that takes the address of this variable. Is that safe?

The old code seems to leak a LOGFONT on the heap every time the function is called. It’s good to fix that if we are confident it was a bug and nothing was relying on that. As far as I can tell from reading documentation it was indeed a mistake and had no benefit.

I don’t think the { 0 } is needed to preserve the old behavior, by the way, because malloc does not zero out the memory it allocates on the heap.

> Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:58
>      memcpy(logFont.lfFaceName, m_name.charactersWithNullTermination().data(), sizeof(logFont.lfFaceName[0]) * std::min<size_t>(static_cast<size_t>(LF_FACESIZE), 1 + m_name.length()));

If we were writing new code, I would suggest using wcscpy_s rather than memcpy here, which would make the code less verbose and do the same thing. Not sure it’s worth changing, though, because this does look correct.

> Source/WebCore/platform/graphics/win/FontPlatformDataWin.cpp:61
> +        auto metrics = MallocPtr<OUTLINETEXTMETRICW>::malloc(bufferSize);

I would use Vector<char, SOME_CONSTANT> here and a typecast to OUTLINETEXTMETRICW*.

> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:1050
> +    auto after = MallocPtr<char>::malloc(bufferLength); // large enough to %-escape every character

Vector<char, SOME_CONSTANT>?

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:283
> +        auto headerBuffer = MallocPtr<char>::malloc(headerBufferLength);

Vector<char, SOME_CONSTANT>?

[Saam Barati]

Comment on attachment 319783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319783&action=review

>>> Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:57
>>> +    LOGFONT logFont { 0 };
>> 
>> There is code below that takes the address of this variable. Is that safe?
> 
> The old code seems to leak a LOGFONT on the heap every time the function is called. It’s good to fix that if we are confident it was a bug and nothing was relying on that. As far as I can tell from reading documentation it was indeed a mistake and had no benefit.
> 
> I don’t think the { 0 } is needed to preserve the old behavior, by the way, because malloc does not zero out the memory it allocates on the heap.

My worry was this code that happens below:
```CGFontCreateWithPlatformFont(&logFont)```

I haven't read that function, but hopefully it doesn't stash a pointer to logFont anywhere, and instead, just uses the value at the memory location.

[Saam Barati]

(In reply to Saam Barati from comment #4)
> Comment on attachment 319783 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319783&action=review
> 
> >>> Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:57
> >>> +    LOGFONT logFont { 0 };
> >> 
> >> There is code below that takes the address of this variable. Is that safe?
> > 
> > The old code seems to leak a LOGFONT on the heap every time the function is called. It’s good to fix that if we are confident it was a bug and nothing was relying on that. As far as I can tell from reading documentation it was indeed a mistake and had no benefit.
> > 
> > I don’t think the { 0 } is needed to preserve the old behavior, by the way, because malloc does not zero out the memory it allocates on the heap.
> 
> My worry was this code that happens below:
> ```CGFontCreateWithPlatformFont(&logFont)```
> 
> I haven't read that function, but hopefully it doesn't stash a pointer to
> logFont anywhere, and instead, just uses the value at the memory location.
This code looks seems scary to me since we're calling into code we don't control as WebKit:
https://developer.apple.com/documentation/coregraphics/1396334-cgfontcreatewithplatformfont

[Yusuke Suzuki]

Comment on attachment 319783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319783&action=review

>>>>> Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:57
>>>>> +    LOGFONT logFont { 0 };
>>>> 
>>>> There is code below that takes the address of this variable. Is that safe?
>>> 
>>> The old code seems to leak a LOGFONT on the heap every time the function is called. It’s good to fix that if we are confident it was a bug and nothing was relying on that. As far as I can tell from reading documentation it was indeed a mistake and had no benefit.
>>> 
>>> I don’t think the { 0 } is needed to preserve the old behavior, by the way, because malloc does not zero out the memory it allocates on the heap.
>> 
>> My worry was this code that happens below:
>> ```CGFontCreateWithPlatformFont(&logFont)```
>> 
>> I haven't read that function, but hopefully it doesn't stash a pointer to logFont anywhere, and instead, just uses the value at the memory location.
> 
> This code looks seems scary to me since we're calling into code we don't control as WebKit:
> https://developer.apple.com/documentation/coregraphics/1396334-cgfontcreatewithplatformfont

CG function description says nothing about this lifetime on Windows...
I assumed that the behavior should be the same to the CreateFontFromLOGFONT since the following code for !USE(CG) uses this.
And CreateFontFromLOGFONT does not manage passed LOGFONT lifetime. Thus the current code leaks memory for !USE(CG) at least.

Should we keep malloced LOGFONT for USE(CG) platform? It potentially leaks memory... And is there any doc about this CG function in Windows?

[Darin Adler]

Comment on attachment 319783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319783&action=review

>>>>>> Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:57
>>>>>> +    LOGFONT logFont { 0 };
>>>>> 
>>>>> There is code below that takes the address of this variable. Is that safe?
>>>> 
>>>> The old code seems to leak a LOGFONT on the heap every time the function is called. It’s good to fix that if we are confident it was a bug and nothing was relying on that. As far as I can tell from reading documentation it was indeed a mistake and had no benefit.
>>>> 
>>>> I don’t think the { 0 } is needed to preserve the old behavior, by the way, because malloc does not zero out the memory it allocates on the heap.
>>> 
>>> My worry was this code that happens below:
>>> ```CGFontCreateWithPlatformFont(&logFont)```
>>> 
>>> I haven't read that function, but hopefully it doesn't stash a pointer to logFont anywhere, and instead, just uses the value at the memory location.
>> 
>> This code looks seems scary to me since we're calling into code we don't control as WebKit:
>> https://developer.apple.com/documentation/coregraphics/1396334-cgfontcreatewithplatformfont
> 
> CG function description says nothing about this lifetime on Windows...
> I assumed that the behavior should be the same to the CreateFontFromLOGFONT since the following code for !USE(CG) uses this.
> And CreateFontFromLOGFONT does not manage passed LOGFONT lifetime. Thus the current code leaks memory for !USE(CG) at least.
> 
> Should we keep malloced LOGFONT for USE(CG) platform? It potentially leaks memory... And is there any doc about this CG function in Windows?

No need for the fear, uncertainty, and doubt. I have access to the source code for that function and it does *not* keep that pointer. This is just what it seems like, a mistake that we can fix. Not something deep and mysterious.

[Yusuke Suzuki]

Comment on attachment 319783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319783&action=review

>> Source/JavaScriptCore/API/JSWrapperMap.mm:79
>> +    auto buffer = MallocPtr<char>::malloc(header + strlen(firstColon + 1) + 1);
> 
> I would not have used MallocPtr<char>::malloc(size) here.
> 
> - I would have suggested Vector<char, SOME_CONSTANT> here instead. Pluses: 1) Avoids heap allocation entirely for common case. 2) Familiar idiom used many places in WebKit. Minuses: 1) Uses more stack space than malloc. 2) Extra overhead to keep track of size and capacity for resizing, which is wasteful in cases where we do not resize. 3) Doesn’t build in type casting for in cases where we need to allocate a certain number of bytes and cast to a pointer type other than "char*". 4) May involve unwanted zeroing of bytes in some cases.
> 
> - My second choice would have been std::make_unique<char[]>(size). Pluses: 1) Built into the standard library. Minuses: 1) Zeroes out the bytes.
> 
> I am not sure why WebKit needs the MallocPtr class unless there is are places we definitely can’t use either of these. I did not check where we are already using it to see why we didn’t use either of my alternatives here.

std::make_unique<char[]>(size)'s problem is that the buffer will be allocated in system malloc instead of bmalloc.

And there is the case that the type of the malloc is completely unrelated to the allocated size. When we call std::make_unique<char[]>(size) or Vector<char> vec(size), the size of the buffer is sizeof(char) * size. But consider the following structure,

struct HeaderWithBuffer {
    ... header ...;
    char trailingBuffer[];
};

auto* buffer = malloc(sizeof(HeaderWithBuffer) + trailingBufferSize);

The above case can be seen in Windows APIs. And it is a bit difficult to handle the above one with std::make_unique / Vector. I think that's why MallocPtr<> is useful.

In the case of this place, Vector<> is enough. I'll change it to Vector here.

>> Source/WTF/wtf/Assertions.cpp:107
>> +        auto buffer = MallocPtr<char>::malloc(length + 1);
> 
> Vector<char, SOME_CONSTANT>?

Changed it to Vector<char, InitialBufferSize = 256>.

>> Source/WTF/wtf/Assertions.cpp:114
>>          CFRelease(cfFormat);
> 
> If we are already touching this code to deal with object lifetimes, we should consider changing these to RetainPtr.

OK, changed.

>> Source/WTF/wtf/Assertions.cpp:135
>> +            auto buffer = MallocPtr<char>::malloc(size);
> 
> Vector<char, SOME_CONSTANT>?
> 
> The way this code silently stops and logs nothing if the allocation fails is something we do not need to preserve.

I think using Vector<char> here is OK since this is not performance critical path.

>> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:141
>> +    auto linkedFonts = MallocPtr<WCHAR>::malloc(linkedFontsBufferSize);
> 
> Vector<WCHAR, SOME_CONSTANT>?

Fixed.

>>>>>>> Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:57
>>>>>>> +    LOGFONT logFont { 0 };
>>>>>> 
>>>>>> There is code below that takes the address of this variable. Is that safe?
>>>>> 
>>>>> The old code seems to leak a LOGFONT on the heap every time the function is called. It’s good to fix that if we are confident it was a bug and nothing was relying on that. As far as I can tell from reading documentation it was indeed a mistake and had no benefit.
>>>>> 
>>>>> I don’t think the { 0 } is needed to preserve the old behavior, by the way, because malloc does not zero out the memory it allocates on the heap.
>>>> 
>>>> My worry was this code that happens below:
>>>> ```CGFontCreateWithPlatformFont(&logFont)```
>>>> 
>>>> I haven't read that function, but hopefully it doesn't stash a pointer to logFont anywhere, and instead, just uses the value at the memory location.
>>> 
>>> This code looks seems scary to me since we're calling into code we don't control as WebKit:
>>> https://developer.apple.com/documentation/coregraphics/1396334-cgfontcreatewithplatformfont
>> 
>> CG function description says nothing about this lifetime on Windows...
>> I assumed that the behavior should be the same to the CreateFontFromLOGFONT since the following code for !USE(CG) uses this.
>> And CreateFontFromLOGFONT does not manage passed LOGFONT lifetime. Thus the current code leaks memory for !USE(CG) at least.
>> 
>> Should we keep malloced LOGFONT for USE(CG) platform? It potentially leaks memory... And is there any doc about this CG function in Windows?
> 
> No need for the fear, uncertainty, and doubt. I have access to the source code for that function and it does *not* keep that pointer. This is just what it seems like, a mistake that we can fix. Not something deep and mysterious.

Great!

>> Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:58
>>      memcpy(logFont.lfFaceName, m_name.charactersWithNullTermination().data(), sizeof(logFont.lfFaceName[0]) * std::min<size_t>(static_cast<size_t>(LF_FACESIZE), 1 + m_name.length()));
> 
> If we were writing new code, I would suggest using wcscpy_s rather than memcpy here, which would make the code less verbose and do the same thing. Not sure it’s worth changing, though, because this does look correct.

I'll leave it for now, since it is not directly related to this patch right now. We should have a patch that performs this refactoring.

>> Source/WebCore/platform/graphics/win/FontPlatformDataWin.cpp:61
>> +        auto metrics = MallocPtr<OUTLINETEXTMETRICW>::malloc(bufferSize);
> 
> I would use Vector<char, SOME_CONSTANT> here and a typecast to OUTLINETEXTMETRICW*.

I think MallocPtr<> is better here. OUTLINETEXTMETRICW's alignment would not be equal to char.

>> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:1050
>> +    auto after = MallocPtr<char>::malloc(bufferLength); // large enough to %-escape every character
> 
> Vector<char, SOME_CONSTANT>?

Fixed.

>> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:283
>> +        auto headerBuffer = MallocPtr<char>::malloc(headerBufferLength);
> 
> Vector<char, SOME_CONSTANT>?

Fixed.

[Yusuke Suzuki]

Created attachment 319835 [details]
Patch for landing

[Darin Adler]

Comment on attachment 319783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319783&action=review

>>> Source/WebCore/platform/graphics/win/FontPlatformDataWin.cpp:61
>>> +        auto metrics = MallocPtr<OUTLINETEXTMETRICW>::malloc(bufferSize);
>> 
>> I would use Vector<char, SOME_CONSTANT> here and a typecast to OUTLINETEXTMETRICW*.
> 
> I think MallocPtr<> is better here. OUTLINETEXTMETRICW's alignment would not be equal to char.

I am pretty sure that Vector<char> guarantees the alignment we need and we rely on that elsewhere too.

[Yusuke Suzuki]

Created attachment 319836 [details]
Patch for landing

[Yusuke Suzuki]

Comment on attachment 319783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319783&action=review

>>>> Source/WebCore/platform/graphics/win/FontPlatformDataWin.cpp:61
>>>> +        auto metrics = MallocPtr<OUTLINETEXTMETRICW>::malloc(bufferSize);
>>> 
>>> I would use Vector<char, SOME_CONSTANT> here and a typecast to OUTLINETEXTMETRICW*.
>> 
>> I think MallocPtr<> is better here. OUTLINETEXTMETRICW's alignment would not be equal to char.
> 
> I am pretty sure that Vector<char> guarantees the alignment we need and we rely on that elsewhere too.

OK, changed to Vector :)

[Yusuke Suzuki]

Created attachment 319837 [details]
Patch for landing

[Yusuke Suzuki]

Committed r221583: <http://trac.webkit.org/changeset/221583>

[Yusuke Suzuki]

Committed r221599: <http://trac.webkit.org/changeset/221599>

[Yoshiaki Jitsukawa]

Created attachment 319879 [details]
Patch to fix compilation errors with HAVE(ISDEBUGGERPRESENT)

[Yusuke Suzuki]

Comment on attachment 319879 [details]
Patch to fix compilation errors with HAVE(ISDEBUGGERPRESENT)

r=me

[Yoshiaki Jitsukawa]

(In reply to Yusuke Suzuki from comment #17)
> Comment on attachment 319879 [details]
> Patch to fix compilation errors with HAVE(ISDEBUGGERPRESENT)
> 
> r=me

Thank you for reviewing! 
Would you reopen this bug so that the commit-queue can work?
I wasn't aware of it and I don't have privilege to do it.

[Yusuke Suzuki]

(In reply to Yoshiaki Jitsukawa from comment #18)
> (In reply to Yusuke Suzuki from comment #17)
> > Comment on attachment 319879 [details]
> > Patch to fix compilation errors with HAVE(ISDEBUGGERPRESENT)
> > 
> > r=me
> 
> Thank you for reviewing! 
> Would you reopen this bug so that the commit-queue can work?
> I wasn't aware of it and I don't have privilege to do it.

Oh, yeah, OK. Could you open a new bug for this and ping me at IRC?
I'll rs=me.

[Yoshiaki Jitsukawa]

(In reply to Yusuke Suzuki from comment #19)
> (In reply to Yoshiaki Jitsukawa from comment #18)
> > (In reply to Yusuke Suzuki from comment #17)
> > > Comment on attachment 319879 [details]
> > > Patch to fix compilation errors with HAVE(ISDEBUGGERPRESENT)
> > > 
> > > r=me
> > 
> > Thank you for reviewing! 
> > Would you reopen this bug so that the commit-queue can work?
> > I wasn't aware of it and I don't have privilege to do it.
> 
> Oh, yeah, OK. Could you open a new bug for this and ping me at IRC?
> I'll rs=me.

Got it. Will do. Thank you!