Closed Bug 431413 Opened 14 years ago Closed 14 years ago

Crash in gfxWindowsFontGroup::InitTextRunUniscribe with testcase from bug 430127

Categories

(Core :: Graphics, defect)

x86
Windows Vista
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: crowderbt, Assigned: crowderbt)

Details

(Keywords: crash, testcase, Whiteboard: [has patch][has review][has approval][sg:critical?] post 1.8-branch)

Attachments

(1 file, 5 obsolete files)

+++ This bug was initially created as a clone of Bug #430127 +++

I am able to get a crash in Uniscribe code with the testcase from the previous bug, but it seems that issue should be handled separately.  This crash seems exploitable.
Flags: blocking1.9?
Attached patch WIP, but broke (obsolete) — Splinter Review
This is a first stab at a patch, but it reveals other problems with the code nearby.  Recovery here is not enough, since when we fall back to GDI we might still fail (thus causing an infinite loop).  More shortly.
Crowder, I'm +ing this, but please add the justification we discussed in IM.
Flags: blocking1.9? → blocking1.9+
Justification is in comment 0: this crash seems to be an exploitable one.
Attached patch no infinite loop (obsolete) — Splinter Review
Attachment #318537 - Attachment is obsolete: true
Attachment #318668 - Flags: review?
Attachment #318668 - Flags: review? → review?(roc)
Comment on attachment 318668 [details] [diff] [review]
no infinite loop

Obviously, the infinite-loop prevention is kludgy.  Let me know if you'd prefer something else.
We should not have an infinite loop here. The first time through the loop, if Uniscribe fails we set mForceGDI to true for the font. The next time through the loop, ShapeGDI and PlaceGDI should not be able to fail. In PlaceGDI, if partialWidthArray.SetLength(mNumGlyphs) fails due to OOM, we should take down the app cleanly --- for 1.9, anyway. It's your call how to do that, there's probably some stdc++ utility function you can call, the same thing that new calls on OOM.

At some point we probably do want to robustify this to handle OOM, but that will probably require preallocating memory or more sophisticated OOM handling, and there's no point in doing that now.
Attached patch cleaner (obsolete) — Splinter Review
Attachment #318668 - Attachment is obsolete: true
Attachment #318716 - Flags: review?(roc)
Attachment #318668 - Flags: review?(roc)
You're right, this doesn't have an infinite loop, and afaict we do eventually crash cleanly afterword.
I'm not comfortable with this, since it leaves this bogus code in 
PlaceGDI:

       if (!partialWidthArray.SetLength(mNumGlyphs))
            return GDI_ERROR;

Which is actually a success code for HRESULT. We should check this since if it fails due to OOM but we start writing to the array, we could easily end up with a stack buffer overrun. So I really think we should abort() here.
why the duplicated chunk of code for the place failure?
Flags: wanted1.8.1.x-
Whiteboard: [sg:critical?] post 1.8-branch
Attached patch another rev (obsolete) — Splinter Review
Sorry about the bad cut and paste.  I feel a little weird about the abort() here, still.
Attachment #318716 - Attachment is obsolete: true
Attachment #318833 - Flags: review?
Attachment #318716 - Flags: review?(roc)
Attachment #318833 - Flags: review? → review?(roc)
-            return GDI_ERROR;
+            abort();

Comment here that we're currently assuming PlaceGDI never fails, so we can't return an error here.

-            NS_ASSERTION(SUCCEEDED(rv), "Failed to shape -- we should never hit this");
+            if (FAILED(rv)) { /* again! */
+                NS_WARNING("Failed to shape, twice -- we should never hit this");
+                aRun->ResetGlyphRuns();
 
-            rv = item->Place();
-            NS_ASSERTION(SUCCEEDED(rv), "Failed to place -- this is pretty bad.");
+                /* Uniscribe doesn't like this font, use GDI instead */
+                item->GetCurrentFont()->GetFontEntry()->mForceGDI = PR_TRUE;
+                break;
+            }
 
+            rv = item->Place();
             if (FAILED(rv)) {
+                NS_WARNING("Failed to place -- this is pretty bad.");
                 aRun->ResetGlyphRuns();
 
                 /* Uniscribe doesn't like this font, use GDI instead */
                 item->GetCurrentFont()->GetFontEntry()->mForceGDI = PR_TRUE;
                 break;

Share this identical error handling block
Crowder, roc is out until Monday.  Is there anyone else who can review this? 
I'll be around enough for the reviews. And I just did review this, waiting for a revised patch.
damon: i can review this (i wrote all the code in question)
Attached patch like so (obsolete) — Splinter Review
Attachment #318833 - Attachment is obsolete: true
Attachment #319038 - Flags: superreview?
Attachment #319038 - Flags: review?(roc)
Attachment #318833 - Flags: review?(roc)
Comment on attachment 319038 [details] [diff] [review]
like so

crowder: Maybe use PR_Abort()

also, use SUCCEEDED() rather than !FAILED()

I would leave both the warnings as assertions and just do the place dependent on the shape succeeding.
Attached patch last one, I hopeSplinter Review
Attachment #319038 - Attachment is obsolete: true
Attachment #319050 - Flags: review?(pavlov)
Attachment #319038 - Flags: superreview?
Attachment #319038 - Flags: review?(roc)
Attachment #319050 - Flags: review?(pavlov) → review+
Comment on attachment 319050 [details] [diff] [review]
last one, I hope

Good enough.

I'd quibble with Stuart over the assertion thing --- people want assertions to be fatal, and in that world, there's no point in asserting something and then testing it. But whatever.
Attachment #319050 - Flags: superreview+
Comment on attachment 319050 [details] [diff] [review]
last one, I hope

Simple, easy fix for a potentially exploitable OOM-handing bug on Windows.
Attachment #319050 - Flags: approval1.9?
I think in the current world we assert for things that really should never ever happen, but then try to deal in the cases that we aren't sure if they might happen anyway and there is a way to deal.
Comment on attachment 319050 [details] [diff] [review]
last one, I hope

Great - let's get this in..
Attachment #319050 - Flags: approval1.9? → approval1.9+
Whiteboard: [sg:critical?] post 1.8-branch → [has patch][has review][has approval][sg:critical?] post 1.8-branch
gfxWindowsFonts.cpp: 1.204
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Product: Core → Core Graveyard
Component: GFX: Win32 → GFX: Thebes
Product: Core Graveyard → Core
QA Contact: win32 → thebes
Group: core-security
The testcase in bug 430127 crashes with an expected OOM abort, so not very useful
as a crash test.
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.