| Summary: | Nullptr crash in objc_msgSend under WebCore::genericFamily | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||
| Component: | Text | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | andersca, darin, ggaren, mmaxfield | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
Created attachment 397335 [details]
patch
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 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 (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? 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. Created attachment 397424 [details]
patch
(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. 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;
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.
I had that when there were multiple returns but went back for simplicity. Committed r260646: <https://trac.webkit.org/changeset/260646> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397424 [details]. (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! Agreed, I was just confused with the claim that I had invented some third way of doing things. |
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