Attachment 343067 Details for Bug 186789 – Patch for landing

[patch] Patch for landing

bug-186789-20180619110740.patch (text/plain), 4.68 KB, created by Jiewen Tan on 2018-06-19 11:07:41 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Jiewen Tan

Created: 2018-06-19 11:07:41 PDT

Size: 4.68 KB

patch

obsolete

>Subversion Revision: 232948
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index a5a0c29194d7b856103d20770be841af1240001a..f7eac0ae4baf710a38b42f0ac451fa96efa2a05b 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,29 @@
>+2018-06-18  Jiewen Tan  <jiewen_tan@apple.com>
>+
>+        Make SecItemShim to not send return value for SecItemAdd
>+        https://bugs.webkit.org/show_bug.cgi?id=186789
>+        <rdar://problem/40892596>
>+
>+        Reviewed by Brent Fulgham.
>+
>+        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 route contradicts the purpose of SecItemShim, which is to proxy all Keychain operations to UIProcess.
>+
>+        Also, this patch removes the release assertion on encode(Encoder&, SecAccessControlRef) and decode(Decoder&, RetainPtr<SecAccessControlRef>&)
>+        as they don't query Keychain.
>+
>+        * Shared/cf/ArgumentCodersCF.cpp:
>+        (IPC::encode):
>+        (IPC::decode):
>+        * Shared/mac/SecItemShim.cpp:
>+        (WebKit::sendSecItemRequest):
>+        (WebKit::webSecItemAdd):
>+        * UIProcess/mac/SecItemShimProxy.cpp:
>+        (WebKit::SecItemShimProxy::secItemRequest):
>+        * UIProcess/mac/SecItemShimProxy.h:
>+        * UIProcess/mac/SecItemShimProxy.messages.in:
>+
> 2018-06-18  Chris Dumez  <cdumez@apple.com>
> 
>         Crash under WebProcessPool::networkProcessFailedToLaunch():
>diff --git a/Source/WebKit/Shared/cf/ArgumentCodersCF.cpp b/Source/WebKit/Shared/cf/ArgumentCodersCF.cpp
>index b430e6cd0b62048f80aa38aa8412c5679d7b7396..69d991d85bf9bb4b70fd1147ef3aee96e0f7d4e4 100644
>--- a/Source/WebKit/Shared/cf/ArgumentCodersCF.cpp
>+++ b/Source/WebKit/Shared/cf/ArgumentCodersCF.cpp
>@@ -771,7 +771,6 @@ bool decode(Decoder& decoder, RetainPtr<SecKeychainItemRef>& result)
> #if HAVE(SEC_ACCESS_CONTROL)
> void encode(Encoder& encoder, SecAccessControlRef accessControl)
> {
>-    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));
>     RetainPtr<CFDataRef> data = adoptCF(SecAccessControlCopyData(accessControl));
>     if (data)
>         encode(encoder, data.get());
>@@ -779,7 +778,6 @@ void encode(Encoder& encoder, SecAccessControlRef accessControl)
> 
> bool decode(Decoder& decoder, RetainPtr<SecAccessControlRef>& result)
> {
>-    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));
>     RetainPtr<CFDataRef> data;
>     if (!decode(decoder, data))
>         return false;
>diff --git a/Source/WebKit/Shared/mac/SecItemShim.cpp b/Source/WebKit/Shared/mac/SecItemShim.cpp
>index 81db3e71e75e1696b15acea8b0387ad56cb973c4..44533a697496621bf7299e42e4b2be9546272414 100644
>--- a/Source/WebKit/Shared/mac/SecItemShim.cpp
>+++ b/Source/WebKit/Shared/mac/SecItemShim.cpp
>@@ -99,14 +99,19 @@ static OSStatus webSecItemCopyMatching(CFDictionaryRef query, CFTypeRef* result)
>     return response->resultCode();
> }
> 
>-static OSStatus webSecItemAdd(CFDictionaryRef query, CFTypeRef* result)
>+static OSStatus webSecItemAdd(CFDictionaryRef query, CFTypeRef* unusedResult)
> {
>+    // 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.
>+    if (unusedResult) {
>+        ASSERT_NOT_REACHED();
>+        return errSecParam;
>+    }
>+
>     auto response = sendSecItemRequest(SecItemRequestData::Add, query);
>     if (!response)
>         return errSecInteractionNotAllowed;
> 
>-    if (result)
>-        *result = response->resultObject().leakRef();
>     return response->resultCode();
> }
> 
>diff --git a/Source/WebKit/UIProcess/mac/SecItemShimProxy.cpp b/Source/WebKit/UIProcess/mac/SecItemShimProxy.cpp
>index 21ea7b012b1898d21012b228725bee106fca6db4..729cfdd2cad89b4620a0721e7730b1733f33b932 100644
>--- a/Source/WebKit/UIProcess/mac/SecItemShimProxy.cpp
>+++ b/Source/WebKit/UIProcess/mac/SecItemShimProxy.cpp
>@@ -76,9 +76,10 @@ void SecItemShimProxy::secItemRequest(const SecItemRequestData& request, SecItem
>     }
> 
>     case SecItemRequestData::Add: {
>-        CFTypeRef resultObject = 0;
>-        OSStatus resultCode = SecItemAdd(request.query(), &resultObject);
>-        response = SecItemResponseData(resultCode, adoptCF(resultObject).get());
>+        // Return value of SecItemAdd is often ignored. Even if it isn't, we don't have the ability to
>+        // serialize SecKeychainItemRef.
>+        OSStatus resultCode = SecItemAdd(request.query(), nullptr);
>+        response = SecItemResponseData(resultCode, nullptr);
>         break;
>     }
>

Flags:

commit-queue: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 186789: 342980 | 343027 | 343067 | 343097