Hi Team, Came across a test case on Twitter posted by an individual, which crashes Safari and Chrome both: Tweet Link - https://twitter.com/yisibl/status/1554691218679861248?s=21&t=z_ZRMNvPNJWLKYRVdVrxAg Test Case: data:text/html;charset=UTF-8,<!doctype html> <style> @font-face { font-family: 'TestFont'; src: local(revert); } </style> <p style="font-family: TestFont">Crash</p> Chrome Bug - https://bugs.chromium.org/p/chromium/issues/detail?id=1342244 Chrome Bug Commit - https://chromium.googlesource.com/chromium/src/+/545aac9815510ec074d78e2a63887a0d59e1b9fa Appreciate if this can be fixed within Safari / Webkit as well. Thanks for your support.
Reproduces. Crash I a null de-ref here: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 WebCore 0x484558f40 WTF::HashTable<WTF::AtomString, WTF::AtomString, WTF::IdentityExtractor, WTF::DefaultHash<WTF::AtomString>, WTF::HashTraits<WTF::AtomString>, WTF::HashTraits<WTF::AtomString> >::add(WTF::AtomString const&) + 48 1 WebCore 0x4851095cf WebCore::FontCache::createFontPlatformData(WebCore::FontDescription const&, WTF::AtomString const&, WebCore::FontCreationContext const&) + 863 2 WebCore 0x485055319 WebCore::FontCache::cachedFontPlatformData(WebCore::FontDescription const&, WTF::String const&, WebCore::FontCreationContext const&, bool) + 825 3 WebCore 0x4846d3eaa WebCore::CSSFontFaceSource::load(WebCore::Document*) + 1082 4 WebCore 0x4846d33e3 WebCore::CSSFontFace::pump(WebCore::ExternalResourceDownloadPolicy) + 339 5 WebCore 0x4846d4521 WebCore::CSSFontFace::font(WebCore::FontDescription const&, bool, bool, WebCore::ExternalResourceDownloadPolicy, WebCore::FontPaletteValues const&) + 129 6 WebCore 0x48470df04 WebCore::CSSFontAccessor::font(WebCore::ExternalResourceDownloadPolicy) const + 84 7 WebCore 0x48470d8c1 WebCore::CSSSegmentedFontFace::fontRanges(WebCore::FontDescription const&, WebCore::FontPaletteValues const&) + 2353 8 WebCore 0x4846e07da WebCore::CSSFontSelector::fontRangesForFamily(WebCore::FontDescription const&, WTF::AtomString const&) + 1018 9 WebCore 0x48506395b decltype(auto) std::__1::__variant_detail::__visitation::__base::__dispatcher<0ul>::__dispatch<std::__1::__variant_detail::__visitation::__variant::__value_visitor<WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_7, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_8>&>&&, std::__1::__variant_detail::__base<(std::__1::__variant_detail::_Trait)1, WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&>(std::__1::__variant_detail::__visitation::__variant::__value_visitor<WTF::Visitor<WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_7, WebCore::realizeNextFallback(WebCore::FontCascadeDescription const&, unsigned int&, WebCore::FontSelector*)::$_8>&>&&, std::__1::__variant_detail::__base<(std::__1::__variant_detail::_Trait)1, WTF::AtomString, WebCore::FontFamilySpecificationCoreText> const&) + 75
<rdar://problem/98062982>
I'm gonna be cautious and turn this into a security bug since we're looking up hash table with null string, which may or may not corrupt the hash table.
#0 0x00000006482747ec in WTF::StringImpl::rawHash() const at /Volumes/Data/safari/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/StringImpl.h:336 #1 0x00000006482747c5 in WTF::StringImpl::hasHash() const at /Volumes/Data/safari/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/StringImpl.h:339 #2 0x000000064827474d in WTF::StringImpl::existingHash() const at /Volumes/Data/safari/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/StringImpl.h:341 #3 0x000000064827471d in WTF::AtomStringHash::hash(WTF::AtomString const&) at /Volumes/Data/safari/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/AtomStringHash.h:39 #4 0x0000000648274065 in unsigned int WTF::IdentityHashTranslator<WTF::HashTraits<WTF::AtomString>, WTF::DefaultHash<WTF::AtomString> >::hash<WTF::AtomString>(WTF::AtomString const&) at /Volumes/Data/safari/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/HashTable.h:311 #5 0x000000064ae5e7ed in WTF::HashTableAddResult<WTF::HashTableIterator<WTF::HashTable<WTF::AtomString, WTF::AtomString, WTF::IdentityExtractor, WTF::DefaultHash<WTF::AtomString>, WTF::HashTraits<WTF::AtomString>, WTF::HashTraits<WTF::AtomString> >, WTF::AtomString, WTF::AtomString, WTF::IdentityExtractor, WTF::DefaultHash<WTF::AtomString>, WTF::HashTraits<WTF::AtomString>, WTF::HashTraits<WTF::AtomString> > > WTF::HashTable<WTF::AtomString, WTF::AtomString, WTF::IdentityExtractor, WTF::DefaultHash<WTF::AtomString>, WTF::HashTraits<WTF::AtomString>, WTF::HashTraits<WTF::AtomString> >::add<WTF::IdentityHashTranslator<WTF::HashTraits<WTF::AtomString>, WTF::DefaultHash<WTF::AtomString> >, WTF::AtomString const&, WTF::AtomString const&>(WTF::AtomString const&, WTF::AtomString const&) at /Volumes/Data/safari/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/HashTable.h:893 #6 0x000000064ae5e6f4 in WTF::HashTable<WTF::AtomString, WTF::AtomString, WTF::IdentityExtractor, WTF::DefaultHash<WTF::AtomString>, WTF::HashTraits<WTF::AtomString>, WTF::HashTraits<WTF::AtomString> >::add(WTF::AtomString const&) at /Volumes/Data/safari/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/HashTable.h:489 #7 0x000000064ae3203b in WTF::HashSet<WTF::AtomString, WTF::DefaultHash<WTF::AtomString>, WTF::HashTraits<WTF::AtomString>, WTF::HashTableTraits>::add(WTF::AtomString const&) at /Volumes/Data/safari/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/HashSet.h:267 #8 0x000000064c92f81b in WebCore::FontCache::shouldAutoActivateFontIfNeeded(WTF::AtomString const&) at /Volumes/Data/safari/OpenSource/Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1301 #9 0x000000064c92fa00 in WebCore::FontCache::createFontPlatformData(WebCore::FontDescription const&, WTF::AtomString const&, WebCore::FontCreationContext const&) at /Volumes/Data/safari/OpenSource/Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1324 #10 0x000000064c765409 in WebCore::FontCache::cachedFontPlatformData(WebCore::FontDescription const&, WTF::String const&, WebCore::FontCreationContext const&, bool) at /Volumes/Data/safari/OpenSource/Source/WebCore/platform/graphics/FontCache.cpp:219 #11 0x000000064c765bc6 in WebCore::FontCache::fontForFamily(WebCore::FontDescription const&, WTF::String const&, WebCore::FontCreationContext const&, bool) at /Volumes/Data/safari/OpenSource/Source/WebCore/platform/graphics/FontCache.cpp:254 #12 0x000000064b20e143 in WebCore::CSSFontFaceSource::load(WebCore::Document*) at /Volumes/Data/safari/OpenSource/Source/WebCore/css/CSSFontFaceSource.cpp:190 #13 0x000000064b20c5fe in WebCore::CSSFontFace::pump(WebCore::ExternalResourceDownloadPolicy) at /Volumes/Data/safari/OpenSource/Source/WebCore/css/CSSFontFace.cpp:590 #14 0x000000064b20e2bf in WebCore::CSSFontFace::font(WebCore::FontDescription const&, bool, bool, WebCore::ExternalResourceDownloadPolicy, WebCore::FontPaletteValues const&) at /Volumes/Data/safari/OpenSource/Source/WebCore/css/CSSFontFace.cpp:654
Geoff/Chris: Can this crash or some variant of it lead to a hash table corruption or not?
Created attachment 461394 [details] Patch
Chris says this is a reliable nullptr so let's reclassify it back to non-security.
Pull request: https://github.com/WebKit/WebKit/pull/3015
Comment on attachment 461394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=461394&action=review r=me with suggestion > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1301 > + return !family.isEmpty() && m_knownFamilies.add(family).isNewEntry; Would you mind doing this a little bit earlier in the function so we avoid unnecessarily doing a remove() from m_knownFamilies.remove() if at capacity?
Comment on attachment 461394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=461394&action=review >> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1301 >> + return !family.isEmpty() && m_knownFamilies.add(family).isNewEntry; > > Would you mind doing this a little bit earlier in the function so we avoid unnecessarily doing a remove() from m_knownFamilies.remove() if at capacity? Good point. Will do.
Created attachment 461405 [details] Patch for landing
Created attachment 461406 [details] Patch for landing
Committed 253127@main (689a3693a796): <https://commits.webkit.org/253127@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 461406 [details].
*** Bug 244294 has been marked as a duplicate of this bug. ***