Bug 231986 – Fix crash when calling setUsernameForLocalCredentialWithID

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[[e-mail]]

rdar://84425279

[[e-mail]]

Created attachment 441803 [details]
Patch

[[e-mail]]

Created attachment 441811 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 441811 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:341
>      auto updatedTag = cbor::CBORWriter::write(cbor::CBORValue(WTFMove(updatedUserMap)));
> -    auto secAttrApplicationTag = [NSData dataWithBytesNoCopy:updatedTag->data() length:updatedTag->size()];
> +
> +    auto secAttrApplicationTag = adoptNS([[NSData alloc] initWithBytes:updatedTag->data() length:updatedTag->size()]);

It's a good idea not to copy memory if you don't have to, but +[NSData dataWithBytesNoCopy:length:] requires that the object that holds the bytes stay alive until the NSData object is deallocated.

This looks like the correct fix.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:344
> -        (__bridge id)kSecAttrApplicationTag: secAttrApplicationTag,
> +        (__bridge id)kSecAttrApplicationTag: secAttrApplicationTag.get(),

In place of using (__bridge id) cast, you can use the new bridge_id_cast() helper function in <wtf/cocoa/TypeCastsCocoa.h>.

This change is not required to land the patch, though.

[[e-mail]]

Created attachment 441819 [details]
Follow up patch to address comment

[[e-mail]]

Follow up is separate in an attempt to keep the history / blame of the file clean, it won't apply cleanly until the first patch is landed (I'll hit retry failed builds then).

[David Kilzer (:ddkilzer)]

Comment on attachment 441819 [details]
Follow up patch to address comment

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

r=me once Apple Cocoa ports build.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:354
> +    status = SecItemUpdate(bridge_cast(query.get()), bridge_cast(updateParams));

For future reference, this:  bridge_cast(query.get())

Can also be written as this:  bridge_cast(query).get()

It doesn't really matter which way you do it now, though it will be more efficient the second way once projects switch to ARC (since we'll be able to tell ARC to just take over the ownership of the `id` object without incrementing the ref count—but we'll have to call .leakRef() or .release() instead of .get() in that specific case).

No need to change this patch to move the .get(), though.

[EWS]

Committed r284574 (243314@main): <https://commits.webkit.org/243314@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441811 [details].

[[e-mail]]

Reopening to queue follow up

[Brent Fulgham]

Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:319:75: error: extraneous ')' before ';'

[Brent Fulgham]

Comment on attachment 441819 [details]
Follow up patch to address comment

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

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:319
> +    NSDictionary *attributes = (__bridge NSDictionary *)attributesArrayRef);

Oops! extra parentheses here!

[[e-mail]]

Putting the follow up patch in a separate bug for good practice: https://bugs.webkit.org/show_bug.cgi?id=232057

[Brent Fulgham]

Comment on attachment 441819 [details]
Follow up patch to address comment

Marking second patch obsolete, since Pascoe is fixing in Bug 232057.