Bug 186009

Summary: [iOS] Fix warnings about leaks found by clang static analyzer
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebCore Misc.Assignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bburg, dbates, ddkilzer, eric.carlson, jbedard, jer.noble, jiewen_tan, joepeck, jonlee, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=185887
https://github.com/sctplab/usrsctp/issues/230
https://github.com/xiph/opus/issues/90
Bug Depends on: 29684    
Bug Blocks:    
Attachments:
Description Flags
Patch v1
none
Patch v2
none
Patch v3 none

Description David Kilzer (:ddkilzer) 2018-05-25 21:39:36 PDT
The clang static analyzer warns leaks in WebKit when compiled for iOS devices.
Comment 1 Radar WebKit Bug Importer 2018-05-25 21:39:50 PDT
<rdar://problem/40574267>
Comment 2 David Kilzer (:ddkilzer) 2018-05-25 21:42:10 PDT
Note that most of these leaks occur in error handling code paths, so they aren't as likely to reproduce.
Comment 3 David Kilzer (:ddkilzer) 2018-05-25 22:12:08 PDT
Created attachment 341389 [details]
Patch v1
Comment 4 David Kilzer (:ddkilzer) 2018-05-25 22:17:10 PDT
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?
Comment 5 David Kilzer (:ddkilzer) 2018-05-25 22:25:22 PDT
Created attachment 341390 [details]
Patch v2
Comment 6 David Kilzer (:ddkilzer) 2018-05-25 22:27:20 PDT
> patch does not apply to trunk of repository

I just rebased it.  Something is wrong with EWS builders when applying patches.
Comment 7 David Kilzer (:ddkilzer) 2018-05-25 22:30:54 PDT
(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
'''
Comment 8 David Kilzer (:ddkilzer) 2018-05-26 08:21:22 PDT
(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.
Comment 9 Daniel Bates 2018-05-26 10:24:03 PDT
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
Comment 10 David Kilzer (:ddkilzer) 2018-05-27 15:18:44 PDT
Created attachment 341431 [details]
Patch v3
Comment 11 David Kilzer (:ddkilzer) 2018-05-27 15:20:19 PDT
(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]).
Comment 12 David Kilzer (:ddkilzer) 2018-05-27 21:23:05 PDT
Committed r232236: <https://trac.webkit.org/changeset/232236>
Comment 13 David Kilzer (:ddkilzer) 2018-05-27 22:14:53 PDT
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