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
David Kilzer (:ddkilzer)
2018-05-25 21:39:36 PDT
Note that most of these leaks occur in error handling code paths, so they aren't as likely to reproduce. Created attachment 341389 [details]
Patch v1
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? Created attachment 341390 [details]
Patch v2
> patch does not apply to trunk of repository
I just rebased it. Something is wrong with EWS builders when applying patches.
(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 ''' (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 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 Created attachment 341431 [details]
Patch v3
(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]). Committed r232236: <https://trac.webkit.org/changeset/232236> 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 |