WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
231986
Fix crash when calling setUsernameForLocalCredentialWithID
https://bugs.webkit.org/show_bug.cgi?id=231986
Summary
Fix crash when calling setUsernameForLocalCredentialWithID
pascoe@apple.com
Reported
2021-10-19 14:53:47 PDT
rdar://84425279
Attachments
Patch
(1.89 KB, patch)
2021-10-19 15:04 PDT
,
pascoe@apple.com
no flags
Details
Formatted Diff
Diff
Patch
(2.04 KB, patch)
2021-10-19 15:56 PDT
,
pascoe@apple.com
no flags
Details
Formatted Diff
Diff
Follow up patch to address comment
(7.51 KB, patch)
2021-10-19 17:09 PDT
,
pascoe@apple.com
bfulgham
: review-
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
pascoe@apple.com
Comment 1
2021-10-19 15:04:11 PDT
Created
attachment 441803
[details]
Patch
pascoe@apple.com
Comment 2
2021-10-19 15:56:59 PDT
Created
attachment 441811
[details]
Patch
David Kilzer (:ddkilzer)
Comment 3
2021-10-19 16:36:10 PDT
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.
pascoe@apple.com
Comment 4
2021-10-19 17:09:04 PDT
Created
attachment 441819
[details]
Follow up patch to address comment
pascoe@apple.com
Comment 5
2021-10-19 17:22:16 PDT
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 6
2021-10-20 14:44:27 PDT
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
Comment 7
2021-10-20 14:49:17 PDT
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]
.
pascoe@apple.com
Comment 8
2021-10-20 14:50:34 PDT
Reopening to queue follow up
Brent Fulgham
Comment 9
2021-10-20 15:33:38 PDT
Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:319:75: error: extraneous ')' before ';'
Brent Fulgham
Comment 10
2021-10-20 15:34:03 PDT
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!
pascoe@apple.com
Comment 11
2021-10-21 07:54:39 PDT
Putting the follow up patch in a separate bug for good practice:
https://bugs.webkit.org/show_bug.cgi?id=232057
Brent Fulgham
Comment 12
2021-10-21 09:26:29 PDT
Comment on
attachment 441819
[details]
Follow up patch to address comment Marking second patch obsolete, since Pascoe is fixing in
Bug 232057
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug