Bug 210456 – dictionaryValueOfType() in WebCoreArgumentCodersMac.mm can be replaced with dynamic_cf_cast<>()

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

dictionaryValueOfType() in WebCoreArgumentCodersMac.mm can be replaced with dynamic_cf_cast<>(CFDictionaryGetValue()).

Currently dictionaryValueOfType() does this:

static CFTypeRef dictionaryValueOfType(CFDictionaryRef dictionary, CFStringRef key, CFTypeID type)
{
    CFTypeRef value = CFDictionaryGetValue(dictionary, key);
    if (value && CFGetTypeID(value) == type)
        return value;
    return nullptr;
}

And dynamic_cf_cast<>() does the same thing, but also adds a Debug assertion when the CFTypeIDs don't match:

template<typename T> T dynamic_cf_cast(CFTypeRef object)
{
    if (!object)
        return nullptr;

    ASSERT_WITH_SECURITY_IMPLICATION(CFGetTypeID(object) == CFTypeTrait<T>::typeID());
    if (CFGetTypeID(object) != CFTypeTrait<T>::typeID())
        return nullptr;

    return static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object));
}

[Darin Adler]

Those aren’t the same.

The dictionaryValueOfType is OK for use when we don’t know the type.

dynamic_cf_cast is only for use when do know the type.

Please don’t collapse them into one function. Feel free to change callers if they are using the wrong one.

Also, dynamic_cf_cast is named wrong. C++'s dynamic_cast is OK when we don’t know the type, so this is more like checked_static_cf_cast.

[David Kilzer (:ddkilzer)]

Created attachment 396337 [details]
Patch v1

[Darin Adler]

Comment on attachment 396337 [details]
Patch v1

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

All of these are cases where it’s OK to assert, because it’s more of a security check, not a real possibility of the wrong type outside a security problem.

> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:132
>      if (protocolProperties)
> -        *protocolProperties = (CFDictionaryRef)dictionaryValueOfType(representation, CFSTR("protocolProperties"), CFDictionaryGetTypeID());
> +        *protocolProperties = dynamic_cf_cast<CFDictionaryRef>(CFDictionaryGetValue(representation, CFSTR("protocolProperties")));

Should return false if the type is wrong.

> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:135
>      if (expectedContentLength)
> -        *expectedContentLength = (CFNumberRef)dictionaryValueOfType(representation, CFSTR("expectedContentLength"), CFNumberGetTypeID());
> +        *expectedContentLength = dynamic_cf_cast<CFNumberRef>(CFDictionaryGetValue(representation, CFSTR("expectedContentLength")));

Ditto.

> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:138
>      if (mimeType)
> -        *mimeType = (CFStringRef)dictionaryValueOfType(representation, CFSTR("mimeType"), CFStringGetTypeID());
> +        *mimeType = dynamic_cf_cast<CFStringRef>(CFDictionaryGetValue(representation, CFSTR("mimeType")));

Ditto.

[David Kilzer (:ddkilzer)]

Comment on attachment 396337 [details]
Patch v1

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

>> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:132
>> +        *protocolProperties = dynamic_cf_cast<CFDictionaryRef>(CFDictionaryGetValue(representation, CFSTR("protocolProperties")));
> 
> Should return false if the type is wrong.

This is what I did to check if the type is wrong:

-    if (protocolProperties)
-        *protocolProperties = dynamic_cf_cast<CFDictionaryRef>(CFDictionaryGetValue(representation, CFSTR("protocolProperties")));
+    if (protocolProperties) {
+        *protocolProperties = nullptr;
+        if (auto value = CFDictionaryGetValue(representation, CFSTR("protocolProperties"))) {
+            auto cfDictionary = dynamic_cf_cast<CFDictionaryRef>(value);
+            if (!cfDictionary)
+                return false;
+            *protocolProperties = cfDictionary;
+        }
+    }

I can't just check the output of dynamic_cf_cast<> because I can't distinguish between passing a nullptr in and passing an object of the wrong type in.

[Darin Adler]

Comment on attachment 396337 [details]
Patch v1

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

>>> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:132
>>> +        *protocolProperties = dynamic_cf_cast<CFDictionaryRef>(CFDictionaryGetValue(representation, CFSTR("protocolProperties")));
>> 
>> Should return false if the type is wrong.
> 
> This is what I did to check if the type is wrong:
> 
> -    if (protocolProperties)
> -        *protocolProperties = dynamic_cf_cast<CFDictionaryRef>(CFDictionaryGetValue(representation, CFSTR("protocolProperties")));
> +    if (protocolProperties) {
> +        *protocolProperties = nullptr;
> +        if (auto value = CFDictionaryGetValue(representation, CFSTR("protocolProperties"))) {
> +            auto cfDictionary = dynamic_cf_cast<CFDictionaryRef>(value);
> +            if (!cfDictionary)
> +                return false;
> +            *protocolProperties = cfDictionary;
> +        }
> +    }
> 
> I can't just check the output of dynamic_cf_cast<> because I can't distinguish between passing a nullptr in and passing an object of the wrong type in.

You should write a helper function since this is going to be done three times! And I think we should check the type even if the caller didn't ask us the fetch it.

    template<typename ValueType> bool extractDictionaryValue(CFString dictionary, CFStringRef key, ValueType* result)
    {
        auto untypedValue = CFDictionaryGetValue(dictionary, key);
        auto value = checked_cf_cast<ValueType>(untypedValue);
        if (untypedValue && !value)
            return false;
        if (result)
            *result = value;
        return true;
    }

   ...

    if (!extractDictionaryValue(representation, CFSTR("protocolProperties"), protocolProperties))
        return false;
    if (!extractDictionaryValue(representation, CFSTR("expectedContentLength"), expectedContentLength))
        return false;
    if (!extractDictionaryValue(representation, CFSTR("mimeType"), mimeType))
        return false;

[David Kilzer (:ddkilzer)]

Comment on attachment 396337 [details]
Patch v1

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

>>>> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:132
>>>> +        *protocolProperties = dynamic_cf_cast<CFDictionaryRef>(CFDictionaryGetValue(representation, CFSTR("protocolProperties")));
>>> 
>>> Should return false if the type is wrong.
>> 
>> This is what I did to check if the type is wrong:
>> 
>> -    if (protocolProperties)
>> -        *protocolProperties = dynamic_cf_cast<CFDictionaryRef>(CFDictionaryGetValue(representation, CFSTR("protocolProperties")));
>> +    if (protocolProperties) {
>> +        *protocolProperties = nullptr;
>> +        if (auto value = CFDictionaryGetValue(representation, CFSTR("protocolProperties"))) {
>> +            auto cfDictionary = dynamic_cf_cast<CFDictionaryRef>(value);
>> +            if (!cfDictionary)
>> +                return false;
>> +            *protocolProperties = cfDictionary;
>> +        }
>> +    }
>> 
>> I can't just check the output of dynamic_cf_cast<> because I can't distinguish between passing a nullptr in and passing an object of the wrong type in.
> 
> You should write a helper function since this is going to be done three times! And I think we should check the type even if the caller didn't ask us the fetch it.
> 
>     template<typename ValueType> bool extractDictionaryValue(CFString dictionary, CFStringRef key, ValueType* result)
>     {
>         auto untypedValue = CFDictionaryGetValue(dictionary, key);
>         auto value = checked_cf_cast<ValueType>(untypedValue);
>         if (untypedValue && !value)
>             return false;
>         if (result)
>             *result = value;
>         return true;
>     }
> 
>    ...
> 
>     if (!extractDictionaryValue(representation, CFSTR("protocolProperties"), protocolProperties))
>         return false;
>     if (!extractDictionaryValue(representation, CFSTR("expectedContentLength"), expectedContentLength))
>         return false;
>     if (!extractDictionaryValue(representation, CFSTR("mimeType"), mimeType))
>         return false;

Note that checked_cf_cast<> crashes (release assert) if the type is wrong.  That would make for less code, but is that what you want here?

Is it okay to assume that protocolProperties, expectedContentLength and mimeType will always be set an never nullptr in the dictionary?  The old code didn't assume that.  Guess I could try to figure out where that comes from.

[Darin Adler]

Comment on attachment 396337 [details]
Patch v1

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

>>>>> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:132
>>>>> +        *protocolProperties = dynamic_cf_cast<CFDictionaryRef>(CFDictionaryGetValue(representation, CFSTR("protocolProperties")));
>>>> 
>>>> Should return false if the type is wrong.
>>> 
>>> This is what I did to check if the type is wrong:
>>> 
>>> -    if (protocolProperties)
>>> -        *protocolProperties = dynamic_cf_cast<CFDictionaryRef>(CFDictionaryGetValue(representation, CFSTR("protocolProperties")));
>>> +    if (protocolProperties) {
>>> +        *protocolProperties = nullptr;
>>> +        if (auto value = CFDictionaryGetValue(representation, CFSTR("protocolProperties"))) {
>>> +            auto cfDictionary = dynamic_cf_cast<CFDictionaryRef>(value);
>>> +            if (!cfDictionary)
>>> +                return false;
>>> +            *protocolProperties = cfDictionary;
>>> +        }
>>> +    }
>>> 
>>> I can't just check the output of dynamic_cf_cast<> because I can't distinguish between passing a nullptr in and passing an object of the wrong type in.
>> 
>> You should write a helper function since this is going to be done three times! And I think we should check the type even if the caller didn't ask us the fetch it.
>> 
>>     template<typename ValueType> bool extractDictionaryValue(CFString dictionary, CFStringRef key, ValueType* result)
>>     {
>>         auto untypedValue = CFDictionaryGetValue(dictionary, key);
>>         auto value = checked_cf_cast<ValueType>(untypedValue);
>>         if (untypedValue && !value)
>>             return false;
>>         if (result)
>>             *result = value;
>>         return true;
>>     }
>> 
>>    ...
>> 
>>     if (!extractDictionaryValue(representation, CFSTR("protocolProperties"), protocolProperties))
>>         return false;
>>     if (!extractDictionaryValue(representation, CFSTR("expectedContentLength"), expectedContentLength))
>>         return false;
>>     if (!extractDictionaryValue(representation, CFSTR("mimeType"), mimeType))
>>         return false;
> 
> Note that checked_cf_cast<> crashes (release assert) if the type is wrong.  That would make for less code, but is that what you want here?
> 
> Is it okay to assume that protocolProperties, expectedContentLength and mimeType will always be set an never nullptr in the dictionary?  The old code didn't assume that.  Guess I could try to figure out where that comes from.

No, I meant dynamic_cf_cast.

[David Kilzer (:ddkilzer)]

Committed r260112: <https://trac.webkit.org/changeset/260112>

[Radar WebKit Bug Importer]

<rdar://problem/61800934>