Bug 202164 – Address static analyzer warnings in ObjCCallbackFunction.mm

• Status: RESOLVED
• Resolution: LATER
• Component: JavaScriptCore

[Keith Rollin]

Xcode's static analyzer reports the following:

    ./API/ObjCCallbackFunction.mm:550:18: warning: Potential leak of an object stored into 'target'
            target = [m_instanceClass alloc];
                     ^

    ./API/ObjCCallbackFunction.mm:701:27: warning: Potential leak of an object of type 'id'
        [invocation setTarget:[target copy]];
                              ^

In the first case, it does seem like target is being leaked. Address this by adding a [target release].

In the second case, management of "target" is a little non-standard. "target" is being copied, with the result being assigned into "invocation" in a non-retained manner. Just looking at this snippet of code, it seems like "target"'s copy is being leaked. However, management of the clone is taken over elsewhere. The line after the "[target copy]" passes "invocation" into a stack of other functions, which take over ownership of the "target" clone. This is explained in some comments above the code cited above, but they weren't quite clear to me. Address this by using "#ifndef __clang_analyzer__" to hide the "[target copy]" from the static analyzer, and adding some more comments to further explain what's going on.

[Radar WebKit Bug Importer]

<rdar://problem/55674457>

[Keith Rollin]

Created attachment 379478 [details]
Patch

[Geoffrey Garen]

Comment on attachment 379478 [details]
Patch

Can we use smart pointers here? Specifically, RetainPtr.

[Tim Horton]

Comment on attachment 379478 [details]
Patch

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

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:556
> +        [target release];

This seems wrong (too early), because anything that uses m_invocation.target after this point is a UAF? Nothing else keeps target alive.

[EWS Watchlist]

Comment on attachment 379478 [details]
Patch

Attachment 379478 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13065005

New failing tests:
apiTests

[Alexey Proskuryakov]

Closing this 5+ years later, as it's an abandoned patch.