578022 - Invalid values in TT's EBLC table leading to heap corruption [@ t2embed!TTDeleteEmbeddedFont]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Christoph Diehl [:posidron]]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; de; rv:1.9.2.6) Gecko/20100625 Firefox/3.6.6
Build Identifier: Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100708 Minefield/4.0b2pre

I wasn't able to determine the exact cause of the bug. During the fuzzing process Firefox always crashed at a random file, I decided to reload one of those files till anything happens. The exceptions occur randomly at one of the ntdll functions. The Firefox release version is not affected.

t2embed!TTDeleteEmbeddedFont
t2embed!T2free
- ntdll!RtlpLowFragHeapFree
- ntdll!RtlpCoalesceFreeBlocks
- ntdll!RtlpFreeHeap

EBLC is located at: 0x14c

Values:
00 00 00 00 # indexTableSize
FF FF FF FF # numberOfIndexSubTables


Reproducible: Always

Steps to Reproduce:
Load the provided html file

Comment 1

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 2

[Christoph Diehl [:posidron]]

Attachment: callstacks

Comment 3

[Daniel Veditz [:dveditz]]

Maybe a bug in windows itself, but can we side-step it? Maybe with the sanitizer proposed in bug 576997 ?

Comment 4

[John Daggett (:jtd)]

The EBLC table is used for embedded bitmaps which I don't think are terribly important to support.  So we could just strip out the table, which Chrome's OTS code does already.

Does this happen on OSX also?  I'm a little confused by the User-Agent string in the description, that appears to be on OSX but the stacks are clearly from Windows.

Comment 5

[Christoph Diehl [:posidron]]

@ John
OSX is not affected. Win7 is running inside a VM.


It turned out that this is not an EBLC specific bug. I noticed the same behavior with the following tables: 

EBDT, GPOS, hhea, name, hmtx, OS/2, FFTM, post 

The procedure to trigger an exception at those tables is basically the same as in the EBLC test. The callstacks of those exceptions are equivalent to the EBLC test.

The same tests against tables like:

EBSC, fpgm, prep, gasp, cvt

did not trigger an exception.

Comment 6

[John Daggett (:jtd)]

(In reply to comment #5)
> It turned out that this is not an EBLC specific bug. I noticed the same
> behavior with the following tables: 
> 
> EBDT, GPOS, hhea, name, hmtx, OS/2, FFTM, post 
> 
> The procedure to trigger an exception at those tables is basically the same as
> in the EBLC test. The callstacks of those exceptions are equivalent to the EBLC
> test.

For these other table types which field are you overwriting?  There isn't the equivalent numberOfIndexSubTables in each of those.  One thing to test is  whether this is caused by using the TTEmbed library or not.  As a fallback we manually revise the name table and load the font using a lower-level API.  Google has found problems in TTEmbed in the past.

Comment 7

[Christoph Diehl [:posidron]]

This is hard to say, because each exception occurs at a random file offset. For example. I overwrote minLeftSideBearing and minRightSideBearing in the hhea table with ff ff ff ff and get an exception @RtlFreeHeap, but the exception is not triggered by those two fields, because in another try it's the field caretSlopeRun. This behavior is the same as for all the tables mentioned above. I also tested the tables in IE 8.0.7600.16385 without a result. What I noticed is that an infinite loop test (like for EBLC) always needs 60 seconds to crash FF. 

I will add a testcase for hhea:
- hhea table is at 0x11c
- value at offset hhea+19 is 80 00 00 00 00 00 00 00

Comment 8

[Christoph Diehl [:posidron]]

Attachment: testcase: hhea

Comment 9

[John Daggett (:jtd)]

Kato-san tracked down the bug.  This looks like a bug in the error-handling logic in gfxGDIFontList::MakePlatformFont.  When loading a TrueType font the code first attempts to load using the t2embed library call.  If that fails it loads using the same path as is used for Postscript CFF fonts *but* since the font is not a CFF font the WinUserFontData will be initialized the wrong way and on descruction the wrong cleanup call will get called (see logic in ~WinUserFontData() at the top of the file).

Kato-san is going to do some more testing, after which I'll try and work up a patch tonight/tomorrow.

Bug 574111 is probably a duplicate of this but I'm not going to mark it as such until this bug is public.

Comment 10

[Jonathan Kew [:jfkthame]]

Argh. Yes, that's bad. The WinUserFontData needs to store the value of isEmbedded rather than isCFF. It looks like I broke this in bug 493280, for no good reason that I can see at this point. :(

I think the fix should be simple: revert WinUserFontData to having an mIsEmbeddedFont field rather than mIsCFF, and pass isEmbedded to the constructor.

Comment 11

[Makoto Kato [:m_kato]]

It is difficult to make a test case for this excepting to Win64 build.  About Win64 binary, it can reproduce by tinderbox's reftest. (bug 574111)

Most situation, 1st chance exception occurs into t2emded.dll.  But this is "1st chance" exception.  A exception handler catches this exception, so process doesn't usually crash.  (2nd chance exception don't occurs on most situation.)

In other word, we can see this 1st chance crash on debugger.

0:000> g
(1964.14a8): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=fffffffe ebx=00000000 ecx=00000000 edx=00000024 esi=685bc8a6 edi=685bcc46
eip=739438c3 esp=001fd634 ebp=001fda10 iopl=0         nv up ei pl zr na pe nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010246
t2embed!TTDeleteEmbeddedFont+0x6c:
739438c3 391f            cmp     dword ptr [edi],ebx  ds:002b:685bcc46=????????


A looped-loading script like Christoph's test case can reproduce this crash on Windows 7. (I cannot reproduce this crash on Vista.  But, of course, 1st change exception always occurs!!).  But, to reproduce this on Windows 7, I spend many times!  It depends on memory layout situation.  So It is difficult to make a test case for this.

Comment 12

[John Daggett (:jtd)]

Confirmed that the bug occurs post-1.9.2, so there's no need to scramble to land a patch for a 3.6.x update.  Naturally, this does affect 4.0 beta1 users.

Comment 13

[John Daggett (:jtd)]

Attachment: patch, v.0.1, delete fonts using the right calls

Corrected the flag passed into WinUserFontData constructor.  Added debug checking for both the TTDeleteEmbeddedFont and RemoveFontMemResourceEx paths.

Also:

- fixed bug in the feature handling that didn't check for a null ptr
- removed forcing CFF fonts to render via GDI on Windows7

Comment 14

[John Daggett (:jtd)]

BTW, I think this should probably block the next beta, not just final.  We should have the patch landed anways within the next few days I imagine.

Comment 15

[John Daggett (:jtd)]

Attachment: patch, v.0.1, delete fonts using the right calls (w/reftest)

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 457765 [details] [diff] [review]
patch, v.0.1, delete fonts using the right calls (w/reftest)

Looks good to me. Just a couple of little style nits I noticed in passing....

+            if (!success) {
+                char buf[256];
+                sprintf(buf, "error deleting font handle (%p) - RemoveFontMemResourceEx failed", mFontRef);
+                NS_ASSERTION(success, buf);
+             }

Nit: closing brace is misaligned.

+            ret = TTLoadEmbeddedFontPtr(&fontRef, TTLOAD_PRIVATE, &privStatus, 
+                                       LICENSE_PREVIEWPRINT, &pulStatus, 
+                                       EOTFontStreamReader::ReadEOTStream, 
+                                       &eotReader, (PRUnichar*)(fontName.get()), 0, 0);

More nits: overlong line (wrap after "&eotReader,"), and there are trailing spaces on these lines which would be good to clean up as you're touching them anyway.

Comment 17

[John Daggett (:jtd)]

Pushed to trunk (edited based on review comments):
http://hg.mozilla.org/mozilla-central/rev/3040b1aebdfa