Bug 151881 – TestNetscapePlugIn: Fix leaks found by static analyzer

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Kilzer (:ddkilzer)]

Fix the following leaks found by the static analyzer in the TestNetscapePlugIn project:

    DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:808:16: warning: Potential leak of memory pointed to by 'path'
            return false;
                   ^~~~~
    DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:808:16: warning: Potential leak of memory pointed to by 'target'
            return false;
                   ^~~~~
    DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:808:16: warning: Potential leak of memory pointed to by 'url'
            return false;
                   ^~~~~

The toCString() method returns malloc()ed memory, and the early returns in testPostURLFile() don't call free() on them.

[David Kilzer (:ddkilzer)]

Created attachment 266649 [details]
Patch v1

[Anders Carlsson]

Comment on attachment 266649 [details]
Patch v1

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

> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:790
> +typedef std::unique_ptr<char, void (*)(void*)> CStringBuffer;
> +static CStringBuffer toCString(const NPString& string)
>  {
> -    char* result = static_cast<char*>(malloc(string.UTF8Length + 1));
> -    memcpy(result, string.UTF8Characters, string.UTF8Length);
> -    result[string.UTF8Length] = '\0';
> +    size_t length = string.UTF8Length;
> +    char* buffer = static_cast<char*>(malloc(length + 1));
> +    if (!buffer)
> +        return CStringBuffer(nullptr, free);
>  
> -    return result;
> +    memcpy(buffer, string.UTF8Characters, length);
> +    buffer[length] = '\0';
> +
> +    return CStringBuffer(buffer, free);

I don't think there's a need for this to use free, can just use std::unique_ptr<char[]> (and std::make_unique).

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> Comment on attachment 266649 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266649&action=review
> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:790
> > +typedef std::unique_ptr<char, void (*)(void*)> CStringBuffer;
> > +static CStringBuffer toCString(const NPString& string)
> >  {
> > -    char* result = static_cast<char*>(malloc(string.UTF8Length + 1));
> > -    memcpy(result, string.UTF8Characters, string.UTF8Length);
> > -    result[string.UTF8Length] = '\0';
> > +    size_t length = string.UTF8Length;
> > +    char* buffer = static_cast<char*>(malloc(length + 1));
> > +    if (!buffer)
> > +        return CStringBuffer(nullptr, free);
> >  
> > -    return result;
> > +    memcpy(buffer, string.UTF8Characters, length);
> > +    buffer[length] = '\0';
> > +
> > +    return CStringBuffer(buffer, free);
> 
> I don't think there's a need for this to use free, can just use
> std::unique_ptr<char[]> (and std::make_unique).

The only downside here is that std::make_unique<> requires <WTF/StdLibExtras.h>, and based on the project sources, I don't think TestNetscapePlugin should depend on WTF.

[Anders Carlsson]

Comment on attachment 266649 [details]
Patch v1

then just use std::unique_ptr<char[]>(new char[string.UTF8Length + 1]);

[David Kilzer (:ddkilzer)]

Created attachment 266669 [details]
Patch v2

[Darin Adler]

Comment on attachment 266669 [details]
Patch v2

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

> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:977
> +    std::unique_ptr<char[]> message(nullptr);

No need for the explicit nullptr here.

[WebKit Commit Bot]

Comment on attachment 266669 [details]
Patch v2

Clearing flags on attachment: 266669

Committed r193607: <http://trac.webkit.org/changeset/193607>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> Comment on attachment 266669 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266669&action=review
> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:977
> > +    std::unique_ptr<char[]> message(nullptr);
> 
> No need for the explicit nullptr here.

Fixed in r193623:  <http://trac.webkit.org/r193623>