WebKit::LocalConnection::createCredentialPrivateKey leaks an NSMutableDictionary when an LAContext object is passed to the method. found by the clang static analyzer.
<rdar://problem/84307054>
Created attachment 441397 [details] Patch v1
This regressed here: Add nil checks for LAContexts before inserting them in the dictionaries. <https://bugs.webkit.org/show_bug.cgi?id=225897> <https://trac.webkit.org/r277658>
Comment on attachment 441397 [details] Patch v1 Marking cq+ since: - ios-sim build is complete, but bubble did not turn green. - mac-AS-debug-wk2 is failing due to some unrelated sandbox issue. - api-mac is a day behind, and based on other tests passing, I don't think this patch will cause any issues.
Committed r284319 (243115@main): <https://commits.webkit.org/243115@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 441397 [details].
Comment on attachment 441397 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=441397&action=review > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:180 > + privateKeyAttributes = adoptNS([privateKeyAttributes mutableCopy]); > + ((NSMutableDictionary *)privateKeyAttributes.get())[(id)kSecUseAuthenticationContext] = context; Not important for this patch, but coding style wise, I think we should write this without the NSMutableDictionary cast: auto mutableCopy = adoptNS([privateKeyAttributes mutableCopy]); mutableCopy.get()[(id)kSecUseAuthenticationContext] = context; privateKeyAttributes = WTFMove(mutableCopy); Might have to be more specific about the type somewhere, but something roughly like that should work.
Reopening to attach new patch.
Created attachment 441547 [details] Patch for follow-up comments
Comment on attachment 441547 [details] Patch for follow-up comments Marking cq+ since this compiled on Cocoa platforms.
Committed r284342 (243136@main): <https://commits.webkit.org/243136@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 441547 [details].
Comment on attachment 441547 [details] Patch for follow-up comments View in context: https://bugs.webkit.org/attachment.cgi?id=441547&action=review > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:181 > + privateKeyAttributes = mutableCopy; No WTFMove? Seems like it would remove one retain/release cycle.
Comment on attachment 441547 [details] Patch for follow-up comments View in context: https://bugs.webkit.org/attachment.cgi?id=441547&action=review >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:181 >> + privateKeyAttributes = mutableCopy; > > No WTFMove? Seems like it would remove one retain/release cycle. I reimplemented your patch (instead of copy-pasting the code), and simply missed the WTMove(). Filed this bug to track catching that in more places: Bug 231954: clang-tidy checker to find places where WTFMove/std::move are missing with RetainPtr<> Will post a follow-up patch in a bit.
Created attachment 441728 [details] Patch for follow-up comments #2
Comment on attachment 441728 [details] Patch for follow-up comments #2 Adding cq+ since it compiles.
Committed r284458 (243218@main): <https://commits.webkit.org/243218@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 441728 [details].