Bug 177848 – REGRESSION (Safari 11): custom <font-face> tag crashes a page

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[metnew]

Created attachment 322618 [details]
PoC

`<font-face>` tag with attrs `font-family` and `font-style="initial"` and `<font-face-name name="anything">` crash page:
Mac, latest Webkit with Asan (from git), tested on Safari Technology Preview too.

==60076==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000115b1d0b6 bp 0x7fff533cfbb0 sp 0x7fff533cfbb0 T0)
==60076==The signal is caused by a READ memory access.
==60076==Hint: address points to the zero page.
==60076==WARNING: invalid path to external symbolizer!
==60076==WARNING: Failed to use and restart external symbolizer!
    #0 0x115b1d0b5 in WebCore::CSSPrimitiveValue::valueID() const (/Users/metnev/Profile/fuzz/webkit/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x1340b5)
    #1 0x115f71c29 in WebCore::calculateItalicRange(WebCore::CSSValue&) (/Users/metnev/Profile/fuzz/webkit/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x588c29)
    #2 0x115f7194b in WebCore::CSSFontFace::setStyle(WebCore::CSSValue&) (/Users/metnev/Profile/fuzz/webkit/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x58894b)
    #3 0x115fa1d23 in WebCore::CSSFontSelector::addFontFaceRule(WebCore::StyleRuleFontFace&, bool) (/Users/metnev/Profile/fuzz/webkit/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x5b8d23)
    #4 0x115fa112b in WebCore::CSSFontSelector::buildCompleted() (/Users/metnev/Profile/fuzz/webkit/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x5b812b)
    #5 0x118a3617b in WebCore::Style::Scope::resolver() (/Users/metnev/Profile/fuzz/webkit/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x304d17b)
    #6 0x118a4fcb6 in WebCore::Style::TreeResolver::Scope::Scope(WebCore::Document&) (/Users/metnev/Profile/fuzz/webkit/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3066cb6)
    #7 0x118a5357e in WebCore::Style::TreeResolver::resolve() (/Users/metnev/Profile/fuzz/webkit/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x306a57e)
    #8 0x1161d7afa in WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) (/Users/metnev/Profile/fuzz/webkit/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x7eeafa)
    #9 0x1161d8ebc in WebCore::Document::updateStyleIfNeeded() (/Users/metnev/Profile/fuzz/webkit/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x7efebc)
    #10 0x118ccd892 in WebCore::ThreadTimers::sharedTimerFiredInternal() (/Users/metnev/Profile/fuzz/webkit/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x32e4892)
    #11 0x117de4d29 in WebCore::timerFired(__CFRunLoopTimer*, void*) (/Users/metnev/Profile/fuzz/webkit/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x23fbd29)
    #12 0x7fffb0f05c53 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64h+0x90c53)
    #13 0x7fffb0f058de in __CFRunLoopDoTimer (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64h+0x908de)
    #14 0x7fffb0f05439 in __CFRunLoopDoTimers (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64h+0x90439)
    #15 0x7fffb0efcb80 in __CFRunLoopRun (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64h+0x87b80)
    #16 0x7fffb0efc113 in CFRunLoopRunSpecific (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64h+0x87113)
    #17 0x7fffb045cebb in RunCurrentEventLoopInMode (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox:x86_64+0x30ebb)
    #18 0x7fffb045ccf0 in ReceiveNextEventCommon (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox:x86_64+0x30cf0)
    #19 0x7fffb045cb25 in _BlockUntilNextEventMatchingListInModeWithFilter (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox:x86_64+0x30b25)
    #20 0x7fffae9f5a53 in _DPSNextEvent (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit:x86_64+0x46a53)
    #21 0x7fffaf1717ed in -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit:x86_64+0x7c27ed)
    #22 0x7fffae9ea3da in -[NSApplication run] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit:x86_64+0x3b3da)
    #23 0x7fffae9b4e0d in NSApplicationMain (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit:x86_64+0x5e0d)
    #24 0x7fffc68dd8c6 in _xpc_objc_main (/usr/lib/system/libxpc.dylib:x86_64+0x108c6)
    #25 0x7fffc68dc2e3 in xpc_main (/usr/lib/system/libxpc.dylib:x86_64+0xf2e3)
    #26 0x10c82d4d6 in main (/Users/metnev/Profile/fuzz/webkit/WebKitBuild/Release/WebKit.framework/Versions/A/XPCServices/com.apple.WebKit.WebContent.xpc/Contents/MacOS/com.apple.WebKit.WebContent.Development:x86_64+0x1000014d6)
    #27 0x7fffc6684234 in start (/usr/lib/system/libdyld.dylib:x86_64+0x5234)

==60076==Register values:
rax = 0x0000b00000000005  rbx = 0x00007fff533cfc20  rcx = 0x0000b00000000007  rdx = 0x0000160000000000  
rdi = 0x0000b00000000001  rsi = 0x0000000119fe0258  rbp = 0x00007fff533cfbb0  rsp = 0x00007fff533cfbb0  
 r8 = 0x0000100000000000   r9 = 0x0000000000000000  r10 = 0x00000fffea679f00  r11 = 0x00000000000001c8  
r12 = 0x0000000119fe0258  r13 = 0x0000000119fe0258  r14 = 0x00007fff533cfbc0  r15 = 0x00007fff533cfbe0  
AddressSanitizer can not provide additional info.

[Radar WebKit Bug Importer]

<rdar://problem/34805168>

[Myles C. Maxfield]

Created attachment 328210 [details]
Patch

[Darin Adler]

Comment on attachment 328210 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        the descriptors shouldn't accept the universal keywords ("initial", "inhert", etc.) and our

typo: "inhert"

> Source/WebCore/svg/SVGFontFaceElement.cpp:66
>      CSSPropertyID propId = cssPropertyIdForSVGAttributeName(name);

As long as we are touching this code, can we just write out the word "property" instead of using "prop"?

> Source/WebCore/svg/SVGFontFaceElement.cpp:69
> +        auto& mutableProperties = m_fontFaceRule->mutableProperties();

I don’t think this local variable name needs the word "mutable" in it.

> Source/WebCore/svg/SVGFontFaceElement.cpp:70
> +        mutableProperties.setProperty(propId, value, false);

There is no need to pass the "false" here, it’s the default.

Should take advantage of the return value of MutableStyleProperties::setProperty. If the function does nothing, then it returns false and we don’t have to do anything further. Could also skip the call to rebuildFontFace in that case.

It’s really inelegant that we let the property get set and then remove it afterward, but I suppose we have to live with that for now.

> Source/WebCore/svg/SVGFontFaceElement.cpp:72
> +        // Because we're using a CSS property parser to parse a descriptor, we have to manually disallow the automatic keywords which all properties accept.

I think the comment needs to go further and emphasize that we let the property get set, and now have to remove it.

> Source/WebCore/svg/SVGFontFaceElement.cpp:74
> +            if (parsedValue->isInheritedValue() || parsedValue->isInitialValue() || parsedValue->isUnsetValue() || parsedValue->isRevertValue())

I worry that this code needs to be updated if we add a new feature such as "revert" and the person working on CSSValue would have no reason to come here and update this code. Because of that, I suggest that instead of an explicit check for these four, you consider adding a function for this purpose in the CSSValue class. Or maybe you could list the types that are OK instead of the types that are forbidden.

> Source/WebCore/svg/SVGFontFaceElement.cpp:75
> +                mutableProperties.setProperty(propId, String(), false);

No need to take advantage of the special case in setProperty, can just call removeProperty directly:

    mutableProperties.removeProperty(propId);

> LayoutTests/svg/text/font-style-keyword.html:11
> +    <font-face font-style="initial" font-family="any">

This tests one of the four keywords. We should at least test the three others. And still that seems like only modest test coverage given that there are separate rules for what is expected for each of the specific attributes.

It is a really loose coupling that CSSFontFace makes assumptions based on how the CSSParser works, but with some layers of software in between. I think it might be even better to fix this by making CSSFontFace resilient to unexpected CSSValue types rather than having some of those functions assume they know exactly what the types are, while others have reason to and so carefully check type.

[Myles C. Maxfield]

Committed r225808: <https://trac.webkit.org/changeset/225808>