REGRESSION (r277658): WebKit::LocalConnection::createCredentialPrivateKey leaks an NSMutableDictionary

• Component: WebXR
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

WebKit::LocalConnection::createCredentialPrivateKey leaks an NSMutableDictionary when an LAContext object is passed to the method.

found by the clang static analyzer.

[Radar WebKit Bug Importer]

<rdar://problem/84307054>

[David Kilzer (:ddkilzer)]

Created attachment 441397 [details]
Patch v1

[David Kilzer (:ddkilzer)]

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>

[David Kilzer (:ddkilzer)]

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.

[EWS]

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].

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

Reopening to attach new patch.

[David Kilzer (:ddkilzer)]

Created attachment 441547 [details]
Patch for follow-up comments

[David Kilzer (:ddkilzer)]

Comment on attachment 441547 [details]
Patch for follow-up comments

Marking cq+ since this compiled on Cocoa platforms.

[EWS]

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].

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Reopening to attach new patch.

[David Kilzer (:ddkilzer)]

Created attachment 441728 [details]
Patch for follow-up comments #2

[David Kilzer (:ddkilzer)]

Comment on attachment 441728 [details]
Patch for follow-up comments #2

Adding cq+ since it compiles.

[EWS]

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].