Bug 186009 – [iOS] Fix warnings about leaks found by clang static analyzer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

The clang static analyzer warns leaks in WebKit when compiled for iOS devices.

[Radar WebKit Bug Importer]

<rdar://problem/40574267>

[David Kilzer (:ddkilzer)]

Note that most of these leaks occur in error handling code paths, so they aren't as likely to reproduce.

[David Kilzer (:ddkilzer)]

Created attachment 341389 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Comment on attachment 341389 [details]
Patch v1

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

> Source/ThirdParty/libwebrtc/ChangeLog:12
> +        * Source/third_party/opus/src/src/opus_compare.c:
> +        * Source/third_party/opus/src/src/opus_demo.c:
> +        (main):
> +        - Free allocated memory on early returns.

Since I removed both of these files from the `opus` target in the libwebrtc Xcode project, maybe we don't care about these changes?

> Source/ThirdParty/libwebrtc/ChangeLog:28
> +        * libwebrtc.xcodeproj/project.pbxproj: Remove opus_compare.c,
> +        opus_demo.c, and repacketizer_demo.c from opus target.  This
> +        code is for stand-alone tools, and although it may be removed
> +        during dead code linking, we don't need to spend time compiling
> +        it.

I debated on whether I should remove these 3 source files completely from the Xcode project, or just remove them from being compiled.  If they're removed from the Xcode project, should they be deleted on disk as well or kept to make future merging easier?  Thoughts?

[David Kilzer (:ddkilzer)]

Created attachment 341390 [details]
Patch v2

[David Kilzer (:ddkilzer)]

> patch does not apply to trunk of repository

I just rebased it.  Something is wrong with EWS builders when applying patches.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #6)
> > patch does not apply to trunk of repository
> 
> I just rebased it.  Something is wrong with EWS builders when applying
> patches.

Hmm.  Looks like a bug in svn-apply:

'''
Parsed 19 diffs from patch file(s).
patching file Source/ThirdParty/libwebrtc/ChangeLog
patching file Source/WebCore/ChangeLog
patching file Source/WebKit/ChangeLog
patching file Source/WebKitLegacy/mac/ChangeLog
patching file Source/ThirdParty/libwebrtc/Source/third_party/opus/src/src/opus_compare.c
patching file Source/ThirdParty/libwebrtc/Source/third_party/opus/src/src/opus_demo.c
patching file Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/user_mbuf.c
patching file Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/user_socket.c
patching file Source/ThirdParty/libwebrtc/WebKit/patch-opus.diff
patch: **** Only garbage was found in the patch input.
rm 'Source/ThirdParty/libwebrtc/WebKit/patch-usrsctp'
patching file Source/ThirdParty/libwebrtc/WebKit/patch-usrsctp.diff
patching file Source/ThirdParty/libwebrtc/libwebrtc.xcodeproj/project.pbxproj
patching file Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm
patching file Source/WebCore/bridge/objc/WebScriptObject.mm
patching file Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm
patching file Source/WebCore/platform/ios/wak/WAKWindow.h
patching file Source/WebKit/UIProcess/Automation/ios/WebAutomationSessionIOS.mm
patching file Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm
patching file Source/WebKitLegacy/mac/WebView/WebHTMLView.mm

Failed to run "[u'/Volumes/Data/Bindings/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /Volumes/Data/Bindings/WebKit
'''

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #7)
> (In reply to David Kilzer (:ddkilzer) from comment #6)
> > > patch does not apply to trunk of repository
> > 
> > I just rebased it.  Something is wrong with EWS builders when applying
> > patches.
> 
> Hmm.  Looks like a bug in svn-apply:
> 
> '''
> Parsed 19 diffs from patch file(s).
> patching file Source/ThirdParty/libwebrtc/ChangeLog
> patching file Source/WebCore/ChangeLog
> patching file Source/WebKit/ChangeLog
> patching file Source/WebKitLegacy/mac/ChangeLog
> patching file
> Source/ThirdParty/libwebrtc/Source/third_party/opus/src/src/opus_compare.c
> patching file
> Source/ThirdParty/libwebrtc/Source/third_party/opus/src/src/opus_demo.c
> patching file
> Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/user_mbuf.c
> patching file
> Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/
> user_socket.c
> patching file Source/ThirdParty/libwebrtc/WebKit/patch-opus.diff
> patch: **** Only garbage was found in the patch input.
> rm 'Source/ThirdParty/libwebrtc/WebKit/patch-usrsctp'
> patching file Source/ThirdParty/libwebrtc/WebKit/patch-usrsctp.diff
> patching file Source/ThirdParty/libwebrtc/libwebrtc.xcodeproj/project.pbxproj
> patching file Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm
> patching file Source/WebCore/bridge/objc/WebScriptObject.mm
> patching file
> Source/WebCore/platform/graphics/avfoundation/objc/
> MediaPlayerPrivateAVFoundationObjC.mm
> patching file Source/WebCore/platform/ios/wak/WAKWindow.h
> patching file
> Source/WebKit/UIProcess/Automation/ios/WebAutomationSessionIOS.mm
> patching file Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm
> patching file Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
> 
> Failed to run "[u'/Volumes/Data/Bindings/WebKit/Tools/Scripts/svn-apply',
> '--force']" exit_code: 2 cwd: /Volumes/Data/Bindings/WebKit
> '''

Hitting Bug 29684.  I have an idea of how to fix it.

[Daniel Bates]

Comment on attachment 341390 [details]
Patch v2

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

> Source/ThirdParty/libwebrtc/Source/third_party/opus/src/src/opus_compare.c:241
> +    free(x);

We should upstream these fixes. For now, it’s fine that you include a patch.

> Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/user_socket.c:2134
> +	struct sockaddr *sa = NULL;

Ditto.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:310
> +                    ASSERT(((NSData *)publicKeyDataRef.get()).length == (1 + 2*ES256KeySizeInBytes)); // 04 | X | Y

Missing spaces around *. Surprised style checker didn’t catch this.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:353
> +                    NSData *nsSignature = (NSData *)signatureRef.get();

Could use auto here since we cast on the right hand side.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:510
> +            NSData *nsSignature = (NSData *)signatureRef.get();

Ditto.

> Source/WebCore/bridge/objc/WebScriptObject.mm:661
> +        sharedUndefined.get() = adoptNS([super allocWithZone:NULL]);

NULL  => nullptr

[David Kilzer (:ddkilzer)]

Created attachment 341431 [details]
Patch v3

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #10)
> Created attachment 341431 [details]
> Patch v3

Checking to see if this patch works on EWS after the fix for Bug 29684 landed.

Also implemented suggestions from Dan's review of Patch v2 (attachment 341390 [details]).

[David Kilzer (:ddkilzer)]

Committed r232236: <https://trac.webkit.org/changeset/232236>

[David Kilzer (:ddkilzer)]

Comment on attachment 341431 [details]
Patch v3

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

> Source/ThirdParty/libwebrtc/ChangeLog:12
> +        * Source/third_party/opus/src/src/opus_compare.c:
> +        * Source/third_party/opus/src/src/opus_demo.c:
> +        (main):
> +        - Free allocated memory on early returns.

Filed:  https://github.com/xiph/opus/issues/90

> Source/ThirdParty/libwebrtc/ChangeLog:16
> +        * Source/third_party/usrsctp/usrsctplib/user_mbuf.c:
> +        (clust_constructor_dup):
> +        (mb_ctor_clust):
> +        - Free allocated memory if `m` is NULL.

The issue in clust_constructor_dup() was fixed upstream by:
https://github.com/sctplab/usrsctp/commit/389aa67d1e18dc2ba72e3cf3761ab1b87370ffef

The same code exists in mb_ctor_clust() upstream, but it is commented out, so it doesn't really matter whether it's fixed at this point.

> Source/ThirdParty/libwebrtc/ChangeLog:19
> +        * Source/third_party/usrsctp/usrsctplib/user_socket.c:
> +        (usrsctp_connect): Free `sa` memory if getsockaddr() returns an
> +        error, but still allocates memory for `sa`.

Filed:  https://github.com/sctplab/usrsctp/issues/230