Bug 201804 – [JSC] REGRESSION (r248938): Leak of uint32_t arrays in testFastForwardCopy32()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Kilzer (:ddkilzer)]

Leak of uint32_t arrays in testFastForwardCopy32() due to `continue` statements not releasing memory allocated by `new`.

void testFastForwardCopy32()
{
#if CPU(X86_64)
    for (const bool aligned : { true, false }) {
        for (const bool overlap : { false, true }) {
            for (size_t arrsize : { 1, 4, 5, 6, 8, 10, 12, 16, 20, 40, 100, 1000}) {
                size_t overlapAmount = 5;

                uint32_t* arr1, *arr2;

                if (overlap) {
                    arr1 = new uint32_t[arrsize * 2];
                    arr2 = arr1 + (arrsize - overlapAmount);
                } else {
                    arr1 = new uint32_t[arrsize];
                    arr2 = new uint32_t[arrsize];
                }

                if (!aligned && arrsize < 3)
                    continue;  // LEAKS!
                if (overlap && arrsize <= overlapAmount + 3)
                    continue;  // LEAKS!

                [...]

                if (!overlap) {
                    delete[] arr1;
                    delete[] arr2;
                } else
                    delete[] arr1;
            }
        }
    }
#endif
}

<https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/b3/testb3_8.cpp>

[David Kilzer (:ddkilzer)]

Leak introduced by:

Bug 200181: Identify memcpy loops in b3
<https://bugs.webkit.org/show_bug.cgi?id=200181>
<https://trac.webkit.org/changeset/248938>

[David Kilzer (:ddkilzer)]

Created attachment 378811 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Created attachment 378812 [details]
Patch v2

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #3)
> Created attachment 378812 [details]
> Patch v2

ERROR: Source/JavaScriptCore/b3/testb3_8.cpp:881:  Use 'WTF::makeUnique<>' instead of 'std::make_unique<>'.  [runtime/wtf_make_unique] [4]
ERROR: Source/JavaScriptCore/b3/testb3_8.cpp:885:  Use 'WTF::makeUnique<>' instead of 'std::make_unique<>'.  [runtime/wtf_make_unique] [4]
ERROR: Source/JavaScriptCore/b3/testb3_8.cpp:886:  Use 'WTF::makeUnique<>' instead of 'std::make_unique<>'.  [runtime/wtf_make_unique] [4]
Total errors found: 3 in 2 files

This is a false positive.  WTF::makeUnique<> doesn't handle POD types, so std::make_unique<> must still be used.  (See build failures of "Patch v1".)

[Radar WebKit Bug Importer]

<rdar://problem/55380614>

[Saam Barati]

Comment on attachment 378812 [details]
Patch v2

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

> Source/JavaScriptCore/b3/testb3_8.cpp:885
> +                    array1 = std::make_unique<uint32_t[]>(arrsize);

Should we be using WTF::makeUnique?

[David Kilzer (:ddkilzer)]

(In reply to Saam Barati from comment #6)
> Comment on attachment 378812 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378812&action=review
> 
> > Source/JavaScriptCore/b3/testb3_8.cpp:885
> > +                    array1 = std::make_unique<uint32_t[]>(arrsize);
> 
> Should we be using WTF::makeUnique?

No.  It doesn't support POD types.  See Comment #4.

[David Kilzer (:ddkilzer)]

Comment on attachment 378812 [details]
Patch v2

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

>>> Source/JavaScriptCore/b3/testb3_8.cpp:885
>>> +                    array1 = std::make_unique<uint32_t[]>(arrsize);
>> 
>> Should we be using WTF::makeUnique?
> 
> No.  It doesn't support POD types.  See Comment #4.

Or is the patch missing a #include of <wtf/UniqueArray.h>?

[Yusuke Suzuki]

Comment on attachment 378812 [details]
Patch v2

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

>>>> Source/JavaScriptCore/b3/testb3_8.cpp:885
>>>> +                    array1 = std::make_unique<uint32_t[]>(arrsize);
>>> 
>>> Should we be using WTF::makeUnique?
>> 
>> No.  It doesn't support POD types.  See Comment #4.
> 
> Or is the patch missing a #include of <wtf/UniqueArray.h>?

Use `makeUniqueArray<uint32_t>(arrsize)` instead.

[David Kilzer (:ddkilzer)]

(In reply to Yusuke Suzuki from comment #9)
> Comment on attachment 378812 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378812&action=review
> 
> >>>> Source/JavaScriptCore/b3/testb3_8.cpp:885
> >>>> +                    array1 = std::make_unique<uint32_t[]>(arrsize);
> >>> 
> >>> Should we be using WTF::makeUnique?
> >> 
> >> No.  It doesn't support POD types.  See Comment #4.
> > 
> > Or is the patch missing a #include of <wtf/UniqueArray.h>?
> 
> Use `makeUniqueArray<uint32_t>(arrsize)` instead.

Thanks!  Just noticed that.

[David Kilzer (:ddkilzer)]

Created attachment 378856 [details]
Patch v3

[David Kilzer (:ddkilzer)]

Created attachment 378858 [details]
Patch v4

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #4)
> (In reply to David Kilzer (:ddkilzer) from comment #3)
> > Created attachment 378812 [details]
> > Patch v2
> 
> ERROR: Source/JavaScriptCore/b3/testb3_8.cpp:881:  Use 'WTF::makeUnique<>'
> instead of 'std::make_unique<>'.  [runtime/wtf_make_unique] [4]
> ERROR: Source/JavaScriptCore/b3/testb3_8.cpp:885:  Use 'WTF::makeUnique<>'
> instead of 'std::make_unique<>'.  [runtime/wtf_make_unique] [4]
> ERROR: Source/JavaScriptCore/b3/testb3_8.cpp:886:  Use 'WTF::makeUnique<>'
> instead of 'std::make_unique<>'.  [runtime/wtf_make_unique] [4]
> Total errors found: 3 in 2 files
> 
> This is a false positive.  WTF::makeUnique<> doesn't handle POD types, so
> std::make_unique<> must still be used.  (See build failures of "Patch v1".)

Filed Bug 201818 to fix this error message when using arrays:

Bug 201818: check-webkit-style: Fix warning message for std::make_unique<typename[]>
<https://bugs.webkit.org/show_bug.cgi?id=201818>

[WebKit Commit Bot]

Comment on attachment 378858 [details]
Patch v4

Clearing flags on attachment: 378858

Committed r249914: <https://trac.webkit.org/changeset/249914>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.