Bug 186789 - Make SecItemShim to not send return value for SecItemAdd
Summary: Make SecItemShim to not send return value for SecItemAdd
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-18 14:42 PDT by Jiewen Tan
Modified: 2018-06-19 15:22 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.35 KB, patch)
2018-06-18 15:37 PDT, Jiewen Tan
bfulgham: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.76 MB, application/zip)
2018-06-19 00:21 PDT, EWS Watchlist
no flags Details
Patch for landing (4.68 KB, patch)
2018-06-19 11:07 PDT, Jiewen Tan
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-02 for mac-sierra (1.28 MB, application/zip)
2018-06-19 14:09 PDT, WebKit Commit Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2018-06-18 14:42:53 PDT
Return value of SecItemAdd is often ignored. Even if it isn't, we don't have the ability to serialize SecKeychainItemRef.Otherwise, it would go through the weird route of serializing SecKeychainItemRef by asking Keychain for its persistent reference. This contradicts the purpose of SecItemShim, which is to proxy all Keychain operations to UIProcess.
Comment 1 Jiewen Tan 2018-06-18 14:43:37 PDT
<rdar://problem/40892596>
Comment 2 Jiewen Tan 2018-06-18 15:37:37 PDT
Created attachment 342980 [details]
Patch
Comment 3 Brent Fulgham 2018-06-18 16:09:35 PDT
Comment on attachment 342980 [details]
Patch

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

r=me with these changes, and with clean EWS.

> Source/WebKit/Shared/mac/SecItemShim.cpp:102
>  static OSStatus webSecItemAdd(CFDictionaryRef query, CFTypeRef* result)

Suggest renaming to "unusedResult"

> Source/WebKit/Shared/mac/SecItemShim.cpp:104
> +    // Return value of SecItemAdd is often ignored. Even if it isn't, we don't have the ability to

// Return value of SecItemAdd should be ignored for WebKit use cases. WebKit can't serialize SecKeychainItemRef, so we do not use it.
// If someone passes a result value to be populated, the API contract is being violated so we should assert.

> Source/WebKit/Shared/mac/SecItemShim.cpp:106
> +    if (result != nullptr) {

if (unusedResult) { ...

> Source/WebKit/Shared/mac/SecItemShim.cpp:113
>          return errSecInteractionNotAllowed;

I also suggest you remove lines 115 and 116, since we don't ever expect to have something here to fill in.
Comment 4 Jiewen Tan 2018-06-18 18:09:21 PDT
Comment on attachment 342980 [details]
Patch

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

Thanks Brent for r+ this patch.

>> Source/WebKit/Shared/mac/SecItemShim.cpp:102
>>  static OSStatus webSecItemAdd(CFDictionaryRef query, CFTypeRef* result)
> 
> Suggest renaming to "unusedResult"

Fixed.

>> Source/WebKit/Shared/mac/SecItemShim.cpp:104
>> +    // Return value of SecItemAdd is often ignored. Even if it isn't, we don't have the ability to
> 
> // Return value of SecItemAdd should be ignored for WebKit use cases. WebKit can't serialize SecKeychainItemRef, so we do not use it.
> // If someone passes a result value to be populated, the API contract is being violated so we should assert.

Fixed.

>> Source/WebKit/Shared/mac/SecItemShim.cpp:106
>> +    if (result != nullptr) {
> 
> if (unusedResult) { ...

Fixed.

>> Source/WebKit/Shared/mac/SecItemShim.cpp:113
>>          return errSecInteractionNotAllowed;
> 
> I also suggest you remove lines 115 and 116, since we don't ever expect to have something here to fill in.

Fixed.
Comment 5 EWS Watchlist 2018-06-19 00:21:19 PDT
Comment on attachment 342980 [details]
Patch

Attachment 342980 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/8244046

New failing tests:
accessibility/mac/selection-notification-focus-change.html
Comment 6 EWS Watchlist 2018-06-19 00:21:20 PDT
Created attachment 343027 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 7 Jiewen Tan 2018-06-19 11:06:27 PDT
The tree of mac-wk2-ews is red. Disregard that result, I am landing the patch.
Comment 8 Jiewen Tan 2018-06-19 11:07:41 PDT
Created attachment 343067 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2018-06-19 14:09:14 PDT
Comment on attachment 343067 [details]
Patch for landing

Rejecting attachment 343067 [details] from commit-queue.

Number of test failures exceeded the failure limit.
Full output: http://webkit-queues.webkit.org/results/8252701
Comment 10 WebKit Commit Bot 2018-06-19 14:09:16 PDT
Created attachment 343097 [details]
Archive of layout-test-results from webkit-cq-02 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-02  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 11 Jiewen Tan 2018-06-19 15:22:20 PDT
Committed r232990: <https://trac.webkit.org/changeset/232990>