210456 2020-04-13 14:34:45 -0700 dictionaryValueOfType() in WebCoreArgumentCodersMac.mm can be replaced with dynamic_cf_cast<>() 2020-04-17 03:09:26 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=210448 https://bugs.webkit.org/show_bug.cgi?id=210519 InRadar P2 Normal --- 210646 1 ddkilzer ddkilzer aestes darin webkit-bug-importer oldest_to_newest 1641112 0 ddkilzer 2020-04-13 14:34:45 -0700 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)); } 1641113 1 darin 2020-04-13 14:37:40 -0700 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. 1641124 2 396337 ddkilzer 2020-04-13 14:52:08 -0700 Created attachment 396337 Patch v1 1641130 3 396337 darin 2020-04-13 15:06:48 -0700 Comment on attachment 396337 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. 1641203 4 396337 ddkilzer 2020-04-13 18:12:53 -0700 Comment on attachment 396337 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. 1641230 5 396337 darin 2020-04-13 20:19:08 -0700 Comment on attachment 396337 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; 1641242 6 396337 ddkilzer 2020-04-13 21:04:25 -0700 Comment on attachment 396337 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. 1641405 7 396337 darin 2020-04-14 09:27:21 -0700 Comment on attachment 396337 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. 1641637 8 ddkilzer 2020-04-14 18:24:43 -0700 Committed r260112: <https://trac.webkit.org/changeset/260112> 1641638 9 webkit-bug-importer 2020-04-14 18:25:15 -0700 <rdar://problem/61800934> 396337 2020-04-13 14:52:08 -0700 2020-04-13 15:06:48 -0700 Patch v1 bug-210456-20200413145300.patch text/plain 4051 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMjYwMDMwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDUxYWM3ZDNiOWRkNDY3NGFk YmNhMGY2ODA3ZDk3MWFkYWJmMDY0NDIuLmFkZmZmMGU5ZjQzM2MzNDkxNmE4NDQyOWFmYzA1MmQz NTEyYTAzZmIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMjAtMDQtMTMgIERhdmlkIEtp bHplciAgPGRka2lsemVyQGFwcGxlLmNvbT4KKworICAgICAgICBkaWN0aW9uYXJ5VmFsdWVPZlR5 cGUoKSBpbiBXZWJDb3JlQXJndW1lbnRDb2RlcnNNYWMubW0gY2FuIGJlIHJlcGxhY2VkIHdpdGgg ZHluYW1pY19jZl9jYXN0PD4oKQorICAgICAgICA8aHR0cHM6Ly93ZWJraXQub3JnL2IvMjEwNDU2 PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogU2hh cmVkL21hYy9XZWJDb3JlQXJndW1lbnRDb2RlcnNNYWMubW06CisgICAgICAgIChJUEM6OmRpY3Rp b25hcnlWYWx1ZU9mVHlwZSk6IERlbGV0ZS4KKyAgICAgICAgKElQQzo6Y3JlYXRlQXJjaGl2ZUxp c3QpOgorICAgICAgICAtIFVzZSBkeW5hbWljX2NmX2Nhc3Q8PihDRkRpY3Rpb25hcnlHZXRWYWx1 ZSgpKSBpbiBwbGFjZQorICAgICAgICAgIG9mIGRpY3Rpb25hcnlWYWx1ZU9mVHlwZSgpLgorCiAy MDIwLTA0LTEzICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHplckBhcHBsZS5jb20+CiAKICAgICAgICAg Y3JlYXRlQXJjaGl2ZUxpc3QoKSBpbiBXZWJDb3JlQXJndW1lbnRDb2RlcnNNYWMubW0gc2hvdWxk IGRvIG1vcmUgdmFsaWRpdHkgY2hlY2tzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L1NoYXJl ZC9tYWMvV2ViQ29yZUFyZ3VtZW50Q29kZXJzTWFjLm1tIGIvU291cmNlL1dlYktpdC9TaGFyZWQv bWFjL1dlYkNvcmVBcmd1bWVudENvZGVyc01hYy5tbQppbmRleCAzMjNlNjc4MjYxZTZiOTlkYzUy YTVkNmY1MDY2OTAzY2E5Yzk3MWY2Li4yMjMwZDhhMDNkMDE3Y2MwMWYwN2I4NzkwZWYwZmEzMjA1 NmJkYTY2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L1NoYXJlZC9tYWMvV2ViQ29yZUFyZ3Vt ZW50Q29kZXJzTWFjLm1tCisrKyBiL1NvdXJjZS9XZWJLaXQvU2hhcmVkL21hYy9XZWJDb3JlQXJn dW1lbnRDb2RlcnNNYWMubW0KQEAgLTEsNSArMSw1IEBACiAvKgotICogQ29weXJpZ2h0IChDKSAy MDEwLTIwMTggQXBwbGUgSW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkLgorICogQ29weXJpZ2h0IChD KSAyMDEwLTIwMjAgQXBwbGUgSW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkLgogICogQ29weXJpZ2h0 IChDKSAyMDEzIENvbXBhbnkgMTAwIEluYy4gQWxsIHJpZ2h0cyByZXNlcnZlZC4KICAqCiAgKiBS ZWRpc3RyaWJ1dGlvbiBhbmQgdXNlIGluIHNvdXJjZSBhbmQgYmluYXJ5IGZvcm1zLCB3aXRoIG9y IHdpdGhvdXQKQEAgLTQwLDYgKzQwLDcgQEAKICNpbXBvcnQgPFdlYkNvcmUvU2VyaWFsaXplZFBs YXRmb3JtRGF0YUN1ZU1hYy5oPgogI2ltcG9ydCA8cGFsL3NwaS9jZi9DRk5ldHdvcmtTUEkuaD4K ICNpbXBvcnQgPHd0Zi9NYWNoU2VuZFJpZ2h0Lmg+CisjaW1wb3J0IDx3dGYvY2YvVHlwZUNhc3Rz Q0YuaD4KIAogI2lmIEVOQUJMRShXSVJFTEVTU19QTEFZQkFDS19UQVJHRVQpCiAjaW1wb3J0IDxX ZWJDb3JlL01lZGlhUGxheWJhY2tUYXJnZXRDb250ZXh0Lmg+CkBAIC05MSwyNCArOTIsMTYgQEAg c3RhdGljIFJldGFpblB0cjxDRk11dGFibGVEaWN0aW9uYXJ5UmVmPiBjcmVhdGVTZXJpYWxpemFi bGVSZXByZXNlbnRhdGlvbihDRkluZGUKICAgICByZXR1cm4gZGljdGlvbmFyeTsKIH0KIAotc3Rh dGljIENGVHlwZVJlZiBkaWN0aW9uYXJ5VmFsdWVPZlR5cGUoQ0ZEaWN0aW9uYXJ5UmVmIGRpY3Rp b25hcnksIENGU3RyaW5nUmVmIGtleSwgQ0ZUeXBlSUQgdHlwZSkKLXsKLSAgICBDRlR5cGVSZWYg dmFsdWUgPSBDRkRpY3Rpb25hcnlHZXRWYWx1ZShkaWN0aW9uYXJ5LCBrZXkpOwotICAgIGlmICh2 YWx1ZSAmJiBDRkdldFR5cGVJRCh2YWx1ZSkgPT0gdHlwZSkKLSAgICAgICAgcmV0dXJuIHZhbHVl OwotICAgIHJldHVybiBudWxscHRyOwotfQotCiBzdGF0aWMgYm9vbCBjcmVhdGVBcmNoaXZlTGlz dChDRkRpY3Rpb25hcnlSZWYgcmVwcmVzZW50YXRpb24sIENGVHlwZVJlZiB0b2tlbk51bGwsIENG SW5kZXgqIHZlcnNpb24sIENGVHlwZVJlZioqIG9iamVjdHMsIENGSW5kZXgqIG9iamVjdENvdW50 LCBDRkRpY3Rpb25hcnlSZWYqIHByb3RvY29sUHJvcGVydGllcywgQ0ZOdW1iZXJSZWYqIGV4cGVj dGVkQ29udGVudExlbmd0aCwgQ0ZTdHJpbmdSZWYqIG1pbWVUeXBlKQogewotICAgIENGTnVtYmVy UmVmIHZlcnNpb25OdW1iZXIgPSAoQ0ZOdW1iZXJSZWYpZGljdGlvbmFyeVZhbHVlT2ZUeXBlKHJl cHJlc2VudGF0aW9uLCBDRlNUUigidmVyc2lvbiIpLCBDRk51bWJlckdldFR5cGVJRCgpKTsKKyAg ICBhdXRvIHZlcnNpb25OdW1iZXIgPSBkeW5hbWljX2NmX2Nhc3Q8Q0ZOdW1iZXJSZWY+KENGRGlj dGlvbmFyeUdldFZhbHVlKHJlcHJlc2VudGF0aW9uLCBDRlNUUigidmVyc2lvbiIpKSk7CiAgICAg aWYgKCF2ZXJzaW9uTnVtYmVyKQogICAgICAgICByZXR1cm4gZmFsc2U7CiAKICAgICBpZiAoIUNG TnVtYmVyR2V0VmFsdWUodmVyc2lvbk51bWJlciwga0NGTnVtYmVyQ0ZJbmRleFR5cGUsIHZlcnNp b24pKQogICAgICAgICByZXR1cm4gZmFsc2U7CiAKLSAgICBDRkFycmF5UmVmIGFyY2hpdmVMaXN0 QXJyYXkgPSAoQ0ZBcnJheVJlZilkaWN0aW9uYXJ5VmFsdWVPZlR5cGUocmVwcmVzZW50YXRpb24s IENGU1RSKCJhcmNoaXZlTGlzdCIpLCBDRkFycmF5R2V0VHlwZUlEKCkpOworICAgIGF1dG8gYXJj aGl2ZUxpc3RBcnJheSA9IGR5bmFtaWNfY2ZfY2FzdDxDRkFycmF5UmVmPihDRkRpY3Rpb25hcnlH ZXRWYWx1ZShyZXByZXNlbnRhdGlvbiwgQ0ZTVFIoImFyY2hpdmVMaXN0IikpKTsKICAgICBpZiAo IWFyY2hpdmVMaXN0QXJyYXkpCiAgICAgICAgIHJldHVybiBmYWxzZTsKIApAQCAtMTM2LDEzICsx MjksMTMgQEAgc3RhdGljIGJvb2wgY3JlYXRlQXJjaGl2ZUxpc3QoQ0ZEaWN0aW9uYXJ5UmVmIHJl cHJlc2VudGF0aW9uLCBDRlR5cGVSZWYgdG9rZW5OdWwKICAgICB9CiAKICAgICBpZiAocHJvdG9j b2xQcm9wZXJ0aWVzKQotICAgICAgICAqcHJvdG9jb2xQcm9wZXJ0aWVzID0gKENGRGljdGlvbmFy eVJlZilkaWN0aW9uYXJ5VmFsdWVPZlR5cGUocmVwcmVzZW50YXRpb24sIENGU1RSKCJwcm90b2Nv bFByb3BlcnRpZXMiKSwgQ0ZEaWN0aW9uYXJ5R2V0VHlwZUlEKCkpOworICAgICAgICAqcHJvdG9j b2xQcm9wZXJ0aWVzID0gZHluYW1pY19jZl9jYXN0PENGRGljdGlvbmFyeVJlZj4oQ0ZEaWN0aW9u YXJ5R2V0VmFsdWUocmVwcmVzZW50YXRpb24sIENGU1RSKCJwcm90b2NvbFByb3BlcnRpZXMiKSkp OwogCiAgICAgaWYgKGV4cGVjdGVkQ29udGVudExlbmd0aCkKLSAgICAgICAgKmV4cGVjdGVkQ29u dGVudExlbmd0aCA9IChDRk51bWJlclJlZilkaWN0aW9uYXJ5VmFsdWVPZlR5cGUocmVwcmVzZW50 YXRpb24sIENGU1RSKCJleHBlY3RlZENvbnRlbnRMZW5ndGgiKSwgQ0ZOdW1iZXJHZXRUeXBlSUQo KSk7CisgICAgICAgICpleHBlY3RlZENvbnRlbnRMZW5ndGggPSBkeW5hbWljX2NmX2Nhc3Q8Q0ZO dW1iZXJSZWY+KENGRGljdGlvbmFyeUdldFZhbHVlKHJlcHJlc2VudGF0aW9uLCBDRlNUUigiZXhw ZWN0ZWRDb250ZW50TGVuZ3RoIikpKTsKIAogICAgIGlmIChtaW1lVHlwZSkKLSAgICAgICAgKm1p bWVUeXBlID0gKENGU3RyaW5nUmVmKWRpY3Rpb25hcnlWYWx1ZU9mVHlwZShyZXByZXNlbnRhdGlv biwgQ0ZTVFIoIm1pbWVUeXBlIiksIENGU3RyaW5nR2V0VHlwZUlEKCkpOworICAgICAgICAqbWlt ZVR5cGUgPSBkeW5hbWljX2NmX2Nhc3Q8Q0ZTdHJpbmdSZWY+KENGRGljdGlvbmFyeUdldFZhbHVl KHJlcHJlc2VudGF0aW9uLCBDRlNUUigibWltZVR5cGUiKSkpOwogCiAgICAgcmV0dXJuIHRydWU7 CiB9Cg==