[webkit-reviews] review granted: [Bug 186789] Make SecItemShim to not send return value for SecItemAdd : [Attachment 342980] 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 186789: Make SecItemShim to not send return value for SecItemAdd
https://bugs.webkit.org/show_bug.cgi?id=186789

Attachment 342980: Patch

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




--- Comment #3 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 342980
  --> https://bugs.webkit.org/attachment.cgi?id=342980
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.