Attachment 370975 Details for Bug 198388 – Patch

[patch] Patch

bug-198388-20190530133028.patch (text/plain), 5.53 KB, created by Chris Dumez on 2019-05-30 13:30:29 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-05-30 13:30:29 PDT

Size: 5.53 KB

patch

obsolete

>Subversion Revision: 245897
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 89da80ea62aa5eb89d51ad097a1b2075b0dcad9d..590bbcaf0a0c28c5b9fc5405372b1bf7b5cbc781 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,32 @@
>+2019-05-30  Chris Dumez  <cdumez@apple.com>
>+
>+        Network process crash when decoding SecItemResponseData
>+        https://bugs.webkit.org/show_bug.cgi?id=198388
>+        <rdar://problem/50408046>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * Shared/cf/ArgumentCodersCF.cpp:
>+        (IPC::decode):
>+        When decoding the elements inside a CFArrayRef, if decoding was successful but
>+        the CFTypeRef element is still null then skip it instead of trying to append it
>+        to the array. A CFArray container is not allowed to contain null.
>+        Some of our decoders for CFTypeRef types may not initialize the element even if
>+        the decode() function returns true. For example, the decoders for CFArrayRef and
>+        CFDictionaryRef return true if the encoded container was null but do not create
>+        a container.
>+
>+        * Shared/mac/SecItemResponseData.cpp:
>+        (WebKit::SecItemResponseData::SecItemResponseData):
>+        nit: The wrong parameter was being moved. This is more efficient.
>+
>+        (WebKit::SecItemResponseData::encode const):
>+        nit: Drop unnecessary .get().
>+
>+        * UIProcess/mac/SecItemShimProxy.cpp:
>+        (WebKit::SecItemShimProxy::secItemRequest):
>+        nit: Use nullptr instead of 0.
>+
> 2019-05-30  Chris Dumez  <cdumez@apple.com>
> 
>         [iOS] Third-party extensions using WKWebView are unable to render anything
>diff --git a/Source/WebKit/Shared/cf/ArgumentCodersCF.cpp b/Source/WebKit/Shared/cf/ArgumentCodersCF.cpp
>index fdb8e88574d3ad53a2eaf32d8aba0b33e098e745..85bebd2a2bf8236e5e2f7824d61a6eb97afdc961 100644
>--- a/Source/WebKit/Shared/cf/ArgumentCodersCF.cpp
>+++ b/Source/WebKit/Shared/cf/ArgumentCodersCF.cpp
>@@ -371,13 +371,16 @@ bool decode(Decoder& decoder, RetainPtr<CFArrayRef>& result)
>     if (!decoder.decode(size))
>         return false;
> 
>-    RetainPtr<CFMutableArrayRef> array = adoptCF(CFArrayCreateMutable(0, 0, &kCFTypeArrayCallBacks));
>+    auto array = adoptCF(CFArrayCreateMutable(0, 0, &kCFTypeArrayCallBacks));
> 
>     for (size_t i = 0; i < size; ++i) {
>         RetainPtr<CFTypeRef> element;
>         if (!decode(decoder, element))
>             return false;
> 
>+        if (!element)
>+            continue;
>+        
>         CFArrayAppendValue(array.get(), element.get());
>     }
> 
>diff --git a/Source/WebKit/Shared/mac/SecItemResponseData.cpp b/Source/WebKit/Shared/mac/SecItemResponseData.cpp
>index 466f6acb6d0db845779fed00595536a0e9d23943..b2534ea2d149167e9bdeee7dad9eb9bdec6ace81 100644
>--- a/Source/WebKit/Shared/mac/SecItemResponseData.cpp
>+++ b/Source/WebKit/Shared/mac/SecItemResponseData.cpp
>@@ -32,15 +32,15 @@
> namespace WebKit {
> 
> SecItemResponseData::SecItemResponseData(OSStatus resultCode, RetainPtr<CFTypeRef>&& resultObject)
>-    : m_resultObject(resultObject)
>-    , m_resultCode(WTFMove(resultCode))
>+    : m_resultObject(WTFMove(resultObject))
>+    , m_resultCode(resultCode)
> {
> }
> 
> void SecItemResponseData::encode(IPC::Encoder& encoder) const
> {
>     encoder << static_cast<int64_t>(m_resultCode);
>-    encoder << static_cast<bool>(m_resultObject.get());
>+    encoder << static_cast<bool>(m_resultObject);
>     if (m_resultObject)
>         IPC::encode(encoder, m_resultObject.get());
> }
>diff --git a/Source/WebKit/UIProcess/mac/SecItemShimProxy.cpp b/Source/WebKit/UIProcess/mac/SecItemShimProxy.cpp
>index d2bb69a85237c8178b745e33ea01db8d71bbdce9..d12bff67a5d891950bb614daad886d195eeeebd4 100644
>--- a/Source/WebKit/UIProcess/mac/SecItemShimProxy.cpp
>+++ b/Source/WebKit/UIProcess/mac/SecItemShimProxy.cpp
>@@ -65,13 +65,13 @@ void SecItemShimProxy::secItemRequest(const SecItemRequestData& request, Complet
>     switch (request.type()) {
>     case SecItemRequestData::Invalid:
>         LOG_ERROR("SecItemShimProxy::secItemRequest received an invalid data request. Please file a bug if you know how you caused this.");
>-        response(SecItemResponseData(errSecParam, nullptr));
>+        response(SecItemResponseData { errSecParam, nullptr });
>         break;
> 
>     case SecItemRequestData::CopyMatching: {
>-        CFTypeRef resultObject = 0;
>+        CFTypeRef resultObject = nullptr;
>         OSStatus resultCode = SecItemCopyMatching(request.query(), &resultObject);
>-        response(SecItemResponseData(resultCode, adoptCF(resultObject).get()));
>+        response(SecItemResponseData { resultCode, adoptCF(resultObject) });
>         break;
>     }
> 
>@@ -79,19 +79,19 @@ void SecItemShimProxy::secItemRequest(const SecItemRequestData& request, Complet
>         // 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));
>+        response(SecItemResponseData { resultCode, nullptr });
>         break;
>     }
> 
>     case SecItemRequestData::Update: {
>         OSStatus resultCode = SecItemUpdate(request.query(), request.attributesToMatch());
>-        response(SecItemResponseData(resultCode, 0));
>+        response(SecItemResponseData { resultCode, nullptr });
>         break;
>     }
> 
>     case SecItemRequestData::Delete: {
>         OSStatus resultCode = SecItemDelete(request.query());
>-        response(SecItemResponseData(resultCode, 0));
>+        response(SecItemResponseData { resultCode, nullptr });
>         break;
>     }
>     }

Actions: View | Formatted Diff | Diff

Attachments on bug 198388: 370975