Closed Bug 578022 Opened 14 years ago Closed 14 years ago

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


(Core :: Graphics, defect)

Windows 7
Not set



Tracking Status
blocking2.0 --- final+


(Reporter: posidron, Assigned: jtd)


(Blocks 1 open bug)


(Keywords: testcase, Whiteboard: [sg:critical])


(4 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; de; rv: 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.

- ntdll!RtlpLowFragHeapFree
- ntdll!RtlpCoalesceFreeBlocks
- ntdll!RtlpFreeHeap

EBLC is located at: 0x14c

00 00 00 00 # indexTableSize
FF FF FF FF # numberOfIndexSubTables

Reproducible: Always

Steps to Reproduce:
Load the provided html file
Attached file testcase
Attached file callstacks
blocking2.0: --- → ?
Whiteboard: [sg:critical]
blocking2.0: ? → final+
Maybe a bug in windows itself, but can we side-step it? Maybe with the sanitizer proposed in bug 576997 ?
Ever confirmed: true
Keywords: testcase
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.
@ 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.
(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.
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
Attached file testcase: hhea
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.
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.
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
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.
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.
Corrected the flag passed into WinUserFontData constructor.  Added debug checking for both the TTDeleteEmbeddedFont and RemoveFontMemResourceEx paths.


- fixed bug in the feature handling that didn't check for a null ptr
- removed forcing CFF fonts to render via GDI on Windows7
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.
Assignee: nobody → jdaggett
Attachment #457760 - Attachment is obsolete: true
Attachment #457765 - Flags: review?(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.
Attachment #457765 - Flags: review?(jfkthame) → review+
Pushed to trunk (edited based on review comments):
Closed: 14 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.