Bug 231814 - REGRESSION (r277658): WebKit::LocalConnection::createCredentialPrivateKey leaks an NSMutableDictionary
Summary: REGRESSION (r277658): WebKit::LocalConnection::createCredentialPrivateKey lea...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebXR (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on: 225897
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-15 10:20 PDT by David Kilzer (:ddkilzer)
Modified: 2021-10-19 09:24 PDT (History)
7 users (show)

See Also:


Attachments
Patch v1 (2.58 KB, patch)
2021-10-15 10:25 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch for follow-up comments (1.85 KB, patch)
2021-10-17 13:13 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch for follow-up comments #2 (1.92 KB, patch)
2021-10-19 08:23 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2021-10-15 10:20:11 PDT
WebKit::LocalConnection::createCredentialPrivateKey leaks an NSMutableDictionary when an LAContext object is passed to the method.

found by the clang static analyzer.
Comment 1 Radar WebKit Bug Importer 2021-10-15 10:20:31 PDT
<rdar://problem/84307054>
Comment 2 David Kilzer (:ddkilzer) 2021-10-15 10:25:23 PDT
Created attachment 441397 [details]
Patch v1
Comment 3 David Kilzer (:ddkilzer) 2021-10-15 10:29:43 PDT
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 4 David Kilzer (:ddkilzer) 2021-10-16 11:13:29 PDT
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.
Comment 5 EWS 2021-10-16 11:17:00 PDT
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 6 Darin Adler 2021-10-16 12:53:45 PDT
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.
Comment 7 David Kilzer (:ddkilzer) 2021-10-17 13:13:15 PDT
Reopening to attach new patch.
Comment 8 David Kilzer (:ddkilzer) 2021-10-17 13:13:17 PDT
Created attachment 441547 [details]
Patch for follow-up comments
Comment 9 David Kilzer (:ddkilzer) 2021-10-17 13:56:03 PDT
Comment on attachment 441547 [details]
Patch for follow-up comments

Marking cq+ since this compiled on Cocoa platforms.
Comment 10 EWS 2021-10-17 14:19:40 PDT
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 11 Darin Adler 2021-10-17 21:25:08 PDT
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 12 David Kilzer (:ddkilzer) 2021-10-19 08:20:05 PDT
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.
Comment 13 David Kilzer (:ddkilzer) 2021-10-19 08:23:04 PDT
Reopening to attach new patch.
Comment 14 David Kilzer (:ddkilzer) 2021-10-19 08:23:06 PDT
Created attachment 441728 [details]
Patch for follow-up comments #2
Comment 15 David Kilzer (:ddkilzer) 2021-10-19 08:28:10 PDT
Comment on attachment 441728 [details]
Patch for follow-up comments #2

Adding cq+ since it compiles.
Comment 16 EWS 2021-10-19 09:24:53 PDT
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].