Bug 210911 – Nullptr crash in objc_msgSend under WebCore::genericFamily

• Component: Text
• Status: RESOLVED FIXED

[Antti Koivisto]

Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x0000000000000018)
[  0] 0x00007fff6fffbc1d libobjc.A.dylib`objc_msgSend + 29

     0x00007fff6fffbc0a:      jne 0x5c83               ; <+131>
     0x00007fff6fffbc0d:  movabsq $0x7ffffffffff8, %r10
     0x00007fff6fffbc17:     andq (%rdi), %r10
     0x00007fff6fffbc1a:     movq %rsi, %r11
 ->  0x00007fff6fffbc1d:     andl 0x18(%r10), %r11d
     0x00007fff6fffbc21:     shlq $0x4, %r11
     0x00007fff6fffbc25:     addq 0x10(%r10), %r11
     0x00007fff6fffbc29:     cmpq (%r11), %rsi
     0x00007fff6fffbc2c:      jne 0x5c38               ; <+56>

[  1] 0x00007fff2f697d01 JavaScriptCore`WTF::String::String(__CFString const*) + 49 at StringCF.cpp:36:20
       32  	{
       33  	    if (!str)
       34  	        return;
       35  	
    -> 36  	    CFIndex size = CFStringGetLength(str);
       37  	    if (size == 0)
       38  	        m_impl = StringImpl::empty();
       39  	    else {
       40  	        Vector<LChar, 1024> lcharBuffer(size);
    
[  2] 0x00007fff3cc506e3 WebCore`WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*) [inlined] void WTF::GenericHashTraits<WTF::String>::assignToEmpty<WTF::String, __CFString const*>(WTF::String&, __CFString const*&&) + 7 at HashTraits.h:72:22
[  2] 0x00007fff3cc506dc WebCore`WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*) [inlined] void WTF::HashMapEnsureTranslator<WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >::KeyValuePairTraits, WTF::StringHash>::translate<WTF::KeyValuePair<WTF::String, WTF::String>, WTF::String const&, WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*)::$_3>(WTF::KeyValuePair<WTF::String, WTF::String>&, WTF::String const&, WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*)::$_3&&) + 144 at HashMap.h:220
[  2] 0x00007fff3cc5064c WebCore`WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*) [inlined] WTF::HashTableAddResult<WTF::HashTableIterator<WTF::String, WTF::KeyValuePair<WTF::String, WTF::String>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::String, WTF::String> >, WTF::StringHash, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >::KeyValuePairTraits, WTF::HashTraits<WTF::String> > > WTF::HashTable<WTF::String, WTF::KeyValuePair<WTF::String, WTF::String>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::String, WTF::String> >, WTF::StringHash, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >::KeyValuePairTraits, WTF::HashTraits<WTF::String> >::add<WTF::HashMapEnsureTranslator<WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >::KeyValuePairTraits, WTF::StringHash>, WTF::String const&, WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*)::$_3>(WTF::String const&, WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*)::$_3&&) + 268 at HashTable.h:994
[  2] 0x00007fff3cc50540 WebCore`WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*) [inlined] WTF::HashTableAddResult<WTF::HashTableIterator<WTF::String, WTF::KeyValuePair<WTF::String, WTF::String>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::String, WTF::String> >, WTF::StringHash, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >::KeyValuePairTraits, WTF::HashTraits<WTF::String> > > WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >::inlineEnsure<WTF::String const&, WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*)::$_3>(WTF::String const&, WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*)::$_3&&) at HashMap.h:367
[  2] 0x00007fff3cc50540 WebCore`WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*) [inlined] WTF::HashTableAddResult<WTF::HashTableIterator<WTF::String, WTF::KeyValuePair<WTF::String, WTF::String>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::String, WTF::String> >, WTF::StringHash, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >::KeyValuePairTraits, WTF::HashTraits<WTF::String> > > WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >::ensure<WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*)::$_3>(WTF::String const&, WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*)::$_3&&) at HashMap.h:423
[  2] 0x00007fff3cc50540 WebCore`WebCore::genericFamily(WTF::String const&, WTF::HashMap<WTF::String, WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >&, __CFString const*) + 32 at SystemFontDatabaseCoreText.cpp:251
[  3] 0x00007fff3cc4da08 WebCore`WebCore::FontDescription::platformResolveGenericFamily(UScriptCode, WTF::AtomString const&, WTF::AtomString const&) + 408 at SystemFontDatabaseCoreText.cpp:0:12
[  4] 0x00007fff3c414406 WebCore`WebCore::CSSFontSelector::fontRangesForFamily(WebCore::FontDescription const&, WTF::AtomString const&)::$_1::operator()() const [inlined] WebCore::resolveGenericFamily(WebCore::Document*, WebCore::FontDescription const&, WTF::AtomString const&) + 24 at CSSFontSelector.cpp:266:27
[  4] 0x00007fff3c4143ee WebCore`WebCore::CSSFontSelector::fontRangesForFamily(WebCore::FontDescription const&, WTF::AtomString const&)::$_1::operator()() const + 46 at CSSFontSelector.cpp:306
[  5] 0x00007fff3c414179 WebCore`WebCore::CSSFontSelector::fontRangesForFamily(WebCore::FontDescription const&, WTF::AtomString const&) + 281 at CSSFontSelector.cpp:322:9
[  6] 0x00007fff3cbd1392 WebCore`WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*) [inlined] WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0::operator()(WTF::AtomString const&) const + 62 at FontCascadeFonts.cpp:149:45
[  6] 0x00007fff3cbd1354 WebCore`WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*) [inlined] WTF::__multi_visitor_return_type<WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const>::__type WTF::__visit_helper<0ul, WTF::__index_sequence<0l> >::__visit<WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const>(WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>&, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&) at Variant.h:1903
[  6] 0x00007fff3cbd1354 WebCore`WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*) [inlined] WTF::__multi_visitor_return_type<WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&>::__type WTF::__visit_helper2<0l, 1ul>::__visit<WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&>(WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>&, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&) at Variant.h:1939
[  6] 0x00007fff3cbd1354 WebCore`WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*) [inlined] WTF::__multi_visitor_return_type<WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&>::__type WTF::__visit_helper2<1l, 1ul>::__visit<WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&>(WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>&, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&) at Variant.h:1940
[  6] 0x00007fff3cbd1354 WebCore`WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*) [inlined] WTF::__multi_visitor_return_type<WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&>::__type WTF::__visit_helper<1ul, WTF::__index_sequence<> >::__visit<WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&>(WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>&, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&) at Variant.h:1977
[  6] 0x00007fff3cbd1354 WebCore`WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*) [inlined] WTF::__multi_visitor_return_type<WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>&, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&>::__type WTF::visit<WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>&, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&>(WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_0, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_1>&, WTF::Variant<WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&) at Variant.h:1989
[  6] 0x00007fff3cbd1354 WebCore`WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*) + 180 at FontCascadeFonts.cpp:160
[  7] 0x00007fff3cbd0d8c WebCore`WebCore::FontCascadeFonts::realizeFallbackRangesAt(WebCore::FontCascadeDescription const&, unsigned int) + 668 at FontCascadeFonts.cpp:186:22
[  8] 0x00007fff3b5420cf WebCore`WebCore::FontCascadeFonts::primaryFont(WebCore::FontCascadeDescription const&) + 47 at FontCascadeFonts.h:130:31
[  9] 0x00007fff3c436dd3 WebCore`WebCore::CSSPrimitiveValue::equalForLengthResolution(WebCore::RenderStyle const&, WebCore::RenderStyle const&) [inlined] WebCore::FontCascade::primaryFont() const + 4 at FontCascade.h:339:21
[  9] 0x00007fff3c436dcf WebCore`WebCore::CSSPrimitiveValue::equalForLengthResolution(WebCore::RenderStyle const&, WebCore::RenderStyle const&) [inlined] WebCore::FontCascade::fontMetrics() const at FontCascade.h:151

[Antti Koivisto]

<rdar://problem/61510208>

[Antti Koivisto]

Created attachment 397335 [details]
patch

[Anders Carlsson]

Comment on attachment 397335 [details]
patch

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

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:253
> +        auto value = adoptCF(CTFontDescriptorCopyAttribute(descriptor.get(), kCTFontFamilyNameAttribute));

Can use dynamic_cf_cast here.

https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/cf/TypeCastsCF.h

[Darin Adler]

Comment on attachment 397335 [details]
patch

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

>> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:253
>> +        auto value = adoptCF(CTFontDescriptorCopyAttribute(descriptor.get(), kCTFontFamilyNameAttribute));
> 
> Can use dynamic_cf_cast here.
> 
> https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/cf/TypeCastsCF.h

Or checked_cf_cast, since we know what the type is supposed to be

[Anders Carlsson]

(In reply to Darin Adler from comment #4)
> Comment on attachment 397335 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397335&action=review
> 
> >> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:253
> >> +        auto value = adoptCF(CTFontDescriptorCopyAttribute(descriptor.get(), kCTFontFamilyNameAttribute));
> > 
> > Can use dynamic_cf_cast here.
> > 
> > https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/cf/TypeCastsCF.h
> 
> Or checked_cf_cast, since we know what the type is supposed to be

Wouldn't that just cause a different crash though?

[Darin Adler]

Comment on attachment 397335 [details]
patch

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

>>>> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:253
>>>> +        auto value = adoptCF(CTFontDescriptorCopyAttribute(descriptor.get(), kCTFontFamilyNameAttribute));
>>> 
>>> Can use dynamic_cf_cast here.
>>> 
>>> https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/cf/TypeCastsCF.h
>> 
>> Or checked_cf_cast, since we know what the type is supposed to be
> 
> Wouldn't that just cause a different crash though?

I’m puzzled about which is correct; if it’s a programming error to have the wrong type, then checked_cf_cast seems with. If it’s not a programming mistake then dynamic_cf_cast is the right thing to use, but asserting is wrong. This patch does ASSERT_NOT_REACHED, so it’s like a third "half bad" case.

[Antti Koivisto]

Created attachment 397424 [details]
patch

[Anders Carlsson]

(In reply to Darin Adler from comment #6)
> Comment on attachment 397335 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397335&action=review
> 
> >>>> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:253
> >>>> +        auto value = adoptCF(CTFontDescriptorCopyAttribute(descriptor.get(), kCTFontFamilyNameAttribute));
> >>> 
> >>> Can use dynamic_cf_cast here.
> >>> 
> >>> https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/cf/TypeCastsCF.h
> >> 
> >> Or checked_cf_cast, since we know what the type is supposed to be
> > 
> > Wouldn't that just cause a different crash though?
> 
> I’m puzzled about which is correct; if it’s a programming error to have the
> wrong type, then checked_cf_cast seems with. If it’s not a programming
> mistake then dynamic_cf_cast is the right thing to use, but asserting is
> wrong. This patch does ASSERT_NOT_REACHED, so it’s like a third "half bad"
> case.

Good point, I missed the ASSERT_NOT_REACHED in the original patch.

[Antti Koivisto]

 dynamic_cf_cast does have debug assert so it seems equivalent to my original patch:

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

[Geoffrey Garen]

Comment on attachment 397424 [details]
patch

r=me

You could declare the lambda "[&]() -> String" to maintain the existing one-liner return. Not sure if we like being that fancy or not.

[Antti Koivisto]

I had that when there were multiple returns but went back for simplicity.

[EWS]

Committed r260646: <https://trac.webkit.org/changeset/260646>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397424 [details].

[Darin Adler]

(In reply to Antti Koivisto from comment #9)
>  dynamic_cf_cast does have debug assert so it seems equivalent to my
> original patch:
> 
>     ASSERT_WITH_SECURITY_IMPLICATION(CFGetTypeID(object) ==
> CFTypeTrait<T>::typeID());
>     if (CFGetTypeID(object) != CFTypeTrait<T>::typeID())
>         return nullptr;

Don’t get me started, though. dynamic_cf_cast is the wrong name for a function if it asserts, because dynamic_cast and dyanmic_objc_cast don’t!

[Antti Koivisto]

Agreed, I was just confused with the claim that I had invented some third way of doing things.