Bug 186789 – Make SecItemShim to not send return value for SecItemAdd

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

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.

[Jiewen Tan]

<rdar://problem/40892596>

[Jiewen Tan]

Created attachment 342980 [details]
Patch

[Brent Fulgham]

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.

[Jiewen Tan]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Jiewen Tan]

The tree of mac-wk2-ews is red. Disregard that result, I am landing the patch.

[Jiewen Tan]

Created attachment 343067 [details]
Patch for landing

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Jiewen Tan]

Committed r232990: <https://trac.webkit.org/changeset/232990>