Bug 71997 – crash: WebCore::FontPlatformData::roundsGlyphAdvances on Lion

• Component: Text
• Status: RESOLVED FIXED

[Kenichi Ishibashi]

Please see a Chromium bug http://crbug.com/98711. We have several crash reports which have the same stack trace.

After investigation, I noticed that the crash happens when following conditions are satisfied.
- Safari ToT or Chromium ToT on Lion
- The page uses webfonts.
- The page has enough length of complex text in which glyphs and characters are not necessary to have one-to-one relation.

Attached a repro that I could make.

The crash is caused by a null reference in ComplexTextController::adjustGlyphsAndAdvances(). In following code,

  bool roundsAdvances = !m_font.isPrinterFont() && fontData->platformData().roundsGlyphAdvances();

fontData is null when the crash happened. The fontData, that is complexTextRuns[r].fontData(), is initialized in ComplexTextController::collectComplexTextRunsForCharactersCoreText() by calling fontCache()->getCachedFontData().

  runFontData = fontCache()->getCachedFontData(m_font.fontDescription(), fontName.get(), false, FontCache::DoNotRetain);
   :
   :
  m_complexTextRuns.append(ComplexTextRun::create(ctRun, runFontData, cp, stringLocation, length, runRange));

The fontCache could return null when NSFontManager can't find a font associated with the given fontName.

An interesting thing is that fontName, which is given by CTFontCopyPostScriptName(), is different between Lion and SnowLeopard. In the attached test case, fontName is "Masterpiece" (the name of webfont the page uses) on Lion, while it is "LastResort" on SnowLeopard. Although the appropriate font name can be obtained by CTFontCopyPostScriptName(), NTFontManager couldn't find the font associated with the given name on Lion.

My suspicion is CoreText has improved dynamic font loading support on Lion, but it's not completely compatible with how WebKit activating custom fonts. I glanced over FontCustomPlatformData.cpp and noticed it uses ATS, which is deprecated according to the document. We might need to update dynamic font activating mechanism and font selecting mechanism, but it seems there is no APIs in CoreText that allow us to do it.

In any case, I think we should add null check for the return value of fontCache()->getCachedFontData(). As far as I tested, it seems just setting fontData to runFontData fixed the crash and the rendering result is the same as SnowLeopard.

[Kenichi Ishibashi]

Created attachment 114430 [details]
A repro case

[Kenichi Ishibashi]

Created attachment 114432 [details]
Proposed patch

Comment on attachment 114432 [details]
Proposed patch

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

> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:263
> +                if (!runFontData)
> +                    runFontData = fontData;

I believe this is wrong, in general (it might work in this case because runFont happened to match fontData, but in general, runFont may be from further down the cascade list). The right thing to do here is to begin by trying to see if runFont matches any of the fonts in the fallback list (you’ll want to get fontDataAt(i) for every i, get the ctFont() and the CGFont from that (see comment in GlyphPageTreeNodeMac.cpp:93 for why), and compare the CGFontRef you get from runFont). If you match, set runFontData to the matching fontData. Only if there’s no match, then you want to look up by name in the font cache (which is then guaranteed to work because you’re not dealing with a custom font).

[Kenichi Ishibashi]

Created attachment 114454 [details]
WIP patch

[Kenichi Ishibashi]

(In reply to comment #3)
> I believe this is wrong, in general (it might work in this case because runFont happened to match fontData, but in general, runFont may be from further down the cascade list). The right thing to do here is to begin by trying to see if runFont matches any of the fonts in the fallback list (you’ll want to get fontDataAt(i) for every i, get the ctFont() and the CGFont from that (see comment in GlyphPageTreeNodeMac.cpp:93 for why), and compare the CGFontRef you get from runFont). If you match, set runFontData to the matching fontData. Only if there’s no match, then you want to look up by name in the font cache (which is then guaranteed to work because you’re not dealing with a custom font).

Thank you so much for detailed explanation. I've revised the patch following your comments. The crash is fixed, but I got many layout test failures. For example, below test in fast/text faild:

  fast/text/fallback-traits-fixup.html = IMAGE+TEXT
  fast/text/international/bidi-neutral-directionality-paragraph-start.html = IMAGE+TEXT
  fast/text/international/complex-character-based-fallback.html = IMAGE+TEXT
  fast/text/international/danda-space.html = IMAGE+TEXT
  fast/text/international/hindi-spacing.html = IMAGE+TEXT
  fast/text/international/hindi-whitespace.html = IMAGE+TEXT
  fast/text/midword-break-before-surrogate-pair.html = IMAGE+TEXT

I think I'm misunderstanding something. Any advices?

Interesting. I will try to apply the patch and figure it out. Thanks for working on this bug!

[Darin Adler]

How is a null dereference a security issue?

[Lucas Forschler]

<rdar://problem/10428449>

[Kenichi Ishibashi]

(In reply to comment #7)
> How is a null dereference a security issue?

It causes crash and crash bugs which assigned to me were marked as security issues so I filed this as a security issue. Please let me know if it's not appropriate.

Thanks,

[Justin Schuh]

(In reply to comment #9)
> (In reply to comment #7)
> > How is a null dereference a security issue?
> 
> It causes crash and crash bugs which assigned to me were marked as security issues so I filed this as a security issue. Please let me know if it's not appropriate.

All crashes are not necessarily security issues. Things like NULL dereferences and resource exhaustion are generally safe crashes. Whereas memory corruption or out-of-band reads are potentially dangerous and should be filed as security issues.

[Kenichi Ishibashi]

(In reply to comment #10)
> All crashes are not necessarily security issues. Things like NULL dereferences and resource exhaustion are generally safe crashes. Whereas memory corruption or out-of-band reads are potentially dangerous and should be filed as security issues.

Got it. Thanks!

(In reply to comment #6)
> Interesting. I will try to apply the patch and figure it out. Thanks for working on this bug!

I have applied attachment 114454 [details] and run the tests, and did not get any failures in fast/text/international or fast/text/fallback-traits-fixup.html. Can you describe what the failures you’re seeing look like?

[Kenichi Ishibashi]

Created attachment 114871 [details]
layout test results

I should have mentioned I run layout tests on SnowLeopard. (I'll try run layout tests on Lion later). I'm still getting some failures in fast/text. Attaching the result of new-run-webkit-tests --debug --pixel-tests fast/text I got.

[Kenichi Ishibashi]

(In reply to comment #13)
> (I'll try run layout tests on Lion later).

I don't see any failures on Lion with new-run-webkit-test --debug fast/text.  When I passed --pixel-tests to NRWT, there were a lot of failures, but I assume these are expected.

Should I enclose the change in ifdefs?

I have applied the patch and run fast/text/international on Mac OS X Snow Leopard 10.6.8 and didn’t get any test failures.

[Kenichi Ishibashi]

Created attachment 115082 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #15)
> I have applied the patch and run fast/text/international on Mac OS X Snow Leopard 10.6.8 and didn’t get any test failures.

Thanks you for confirmation. Looks like there is something wrong with my machine settings. Updated the patch for review (just added ChangeLog).

[Kenichi Ishibashi]

Could you take another look, mitz?

Comment on attachment 115082 [details]
Patch

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

Thanks for the patch! It’s good, but you should fix the base character issue.

> Source/WebCore/ChangeLog:8
> +        Avoid setting invalid fontData to ComplexTextRun.

It would be nice to give a fuller description of what caused this bug and how it’s addressed here.

> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:259
> +                    runFontData = candidateFontData->fontDataForCharacter(runStartCharacter);

The purpose of this loop is to match against one of the fonts in the WebCascadeList we’ve created, therefore we should be using the same character used in the WebCascadeList (currently in the baseCharacter variable, which is not in scope here), not the first character of the run.

[Kenichi Ishibashi]

Created attachment 115743 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 115082 [details]
Patch

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

I addressed your comments. Thank you for the review!

>> Source/WebCore/ChangeLog:8
>> +        Avoid setting invalid fontData to ComplexTextRun.
> 
> It would be nice to give a fuller description of what caused this bug and how it’s addressed here.

Done.

>> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:259
>> +                    runFontData = candidateFontData->fontDataForCharacter(runStartCharacter);
> 
> The purpose of this loop is to match against one of the fonts in the WebCascadeList we’ve created, therefore we should be using the same character used in the WebCascadeList (currently in the baseCharacter variable, which is not in scope here), not the first character of the run.

Done.

Comment on attachment 115743 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        fontCache, but it could be null when the font is in fallback
> +        list. Before looking up the fontCache, try to see whether the font

The reason a font from the fallback list might not be found in the font cache is that it may be a web font.

> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:190
> +    U16_GET(cp, 0, 0, length, baseCharacter);

This didn’t have to move out of the if (fontData == systemFallbackFontData()) block, since baseCharacter is used only if we took that branch. But since this initialization is not very expensive, it’s also okay to do it here.

[Kenichi Ishibashi]

Created attachment 115744 [details]
Patch for landing

[Kenichi Ishibashi]

Comment on attachment 115743 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        list. Before looking up the fontCache, try to see whether the font
> 
> The reason a font from the fallback list might not be found in the font cache is that it may be a web font.

Done.

>> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:190
>> +    U16_GET(cp, 0, 0, length, baseCharacter);
> 
> This didn’t have to move out of the if (fontData == systemFallbackFontData()) block, since baseCharacter is used only if we took that branch. But since this initialization is not very expensive, it’s also okay to do it here.

Move into the if (fontData == systemFallbackFontData()) block.

[WebKit Review Bot]

Comment on attachment 115744 [details]
Patch for landing

Clearing flags on attachment: 115744

Committed r100728: <http://trac.webkit.org/changeset/100728>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenichi Ishibashi]

> UChar32 baseCharacter;

This makes a warning.

[Mihnea Ovidenie]

(In reply to comment #27)
> > UChar32 baseCharacter;
> 
> This makes a warning.

Fixed in http://trac.webkit.org/changeset/100765.