Bug 243500

Summary:

Nullptr crash in FontCache::shouldAutoActivateFontIfNeeded

Product:

WebKit

Reporter:

Ahmad Saleem <ahmad.saleem792>

Component:

Text

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, bfulgham, cdumez, ews-feeder, ggaren, gsnedders, koivisto, mmaxfield, rniwa, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

macOS 12

Attachments:

Description	Flags
Patch	none
Patch for landing	ews-feeder: commit-queue-
Patch for landing	none

Ahmad Saleem

Reported 2022-08-03 10:12:56 PDT

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.

Attachments
Patch (2.69 KB, patch) 2022-08-04 00:05 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch for landing (2.64 KB, patch) 2022-08-04 11:52 PDT, Ryosuke Niwa	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch for landing (2.65 KB, patch) 2022-08-04 12:05 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Simon Fraser (smfr)

Comment 1 2022-08-03 10:40:58 PDT

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

Radar WebKit Bug Importer

Comment 2 2022-08-03 10:41:11 PDT

<rdar://problem/98062982>

Ryosuke Niwa

Comment 3 2022-08-03 23:29:21 PDT

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.

Ryosuke Niwa

Comment 4 2022-08-03 23:29:38 PDT

#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

Ryosuke Niwa

Comment 5 2022-08-03 23:32:26 PDT

Geoff/Chris: Can this crash or some variant of it lead to a hash table corruption or not?

Ryosuke Niwa

Comment 6 2022-08-04 00:05:48 PDT

Created attachment 461394 [details] Patch

Ryosuke Niwa

Comment 7 2022-08-04 10:42:38 PDT

Chris says this is a reliable nullptr so let's reclassify it back to non-security.

Ryosuke Niwa

Comment 8 2022-08-04 10:46:52 PDT

Pull request: https://github.com/WebKit/WebKit/pull/3015

Chris Dumez

Comment 9 2022-08-04 10:58:51 PDT

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?

Ryosuke Niwa

Comment 10 2022-08-04 11:01:01 PDT

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.

Ryosuke Niwa

Comment 11 2022-08-04 11:52:21 PDT

Created attachment 461405 [details] Patch for landing

Ryosuke Niwa

Comment 12 2022-08-04 12:05:47 PDT

Created attachment 461406 [details] Patch for landing

EWS

Comment 13 2022-08-04 13:07:08 PDT

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].

Sam Sneddon [:gsnedders]

Comment 14 2022-09-01 08:46:41 PDT

*** Bug 244294 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.