[webkit-reviews] review granted: [Bug 219709] [WebAuthn] Adopt new UI for the Platform Authenticator makeCredential flow : [Attachment 415970] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 219709: [WebAuthn] Adopt new UI for the Platform Authenticator
makeCredential flow
https://bugs.webkit.org/show_bug.cgi?id=219709

Attachment 415970: Patch

https://bugs.webkit.org/attachment.cgi?id=415970&action=review




--- Comment #6 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 415970
  --> https://bugs.webkit.org/attachment.cgi?id=415970
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415970&action=review

r=me

>>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:315
>>> +	 SecAccessControlRef accessControlRef = accessControl.get();
>> 
>> Does accessControlRef need to be retained in some fashion? Will calling
'verifyUser' with this be safe?
> 
> Yes, it is retained in the callback.

Ah! Gotcha, My mistake!

>>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:119
>>> +	 [context evaluateAccessControl:accessControl
operation:LAAccessControlOperationUseKeySign options:options.get()
reply:reply.get()];
>> 
>> This method call seems to use 'accessControl', which is not guaranteed to
still live since you moved the underlying object in the calling method.
> 
> accessControl is held by the completionHandler, which is the held by reply.
As long as the callee doesn't destruct the reply before verifying the
accessControl, it should be safe.

Got it.