431413 - Crash in gfxWindowsFontGroup::InitTextRunUniscribe with testcase from bug 430127

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Brian Crowder]

+++ 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.

Comment 1

[Brian Crowder]

Attachment: WIP, but broke

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.

Comment 2

[Damon Sicore (:damons)]

Crowder, I'm +ing this, but please add the justification we discussed in IM.

Comment 3

[Brian Crowder]

Justification is in comment 0: this crash seems to be an exploitable one.

Comment 4

[Brian Crowder]

Attachment: no infinite loop

Comment 5

[Brian Crowder]

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[Brian Crowder]

Attachment: cleaner

Comment 8

[Brian Crowder]

You're right, this doesn't have an infinite loop, and afaict we do eventually crash cleanly afterword.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[Stuart Parmenter]

why the duplicated chunk of code for the place failure?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, fix that too!

Comment 12

[Brian Crowder]

Attachment: another rev

Sorry about the bad cut and paste.  I feel a little weird about the abort() here, still.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-            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

Comment 14

[Damon Sicore (:damons)]

Crowder, roc is out until Monday.  Is there anyone else who can review this? 

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll be around enough for the reviews. And I just did review this, waiting for a revised patch.

Comment 16

[Stuart Parmenter]

damon: i can review this (i wrote all the code in question)

Comment 17

[Brian Crowder]

Attachment: like so

Comment 18

[Stuart Parmenter]

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.

Comment 19

[Brian Crowder]

Attachment: last one, I hope

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 21

[Brian Crowder]

Comment on attachment 319050 [details] [diff] [review]
last one, I hope

Simple, easy fix for a potentially exploitable OOM-handing bug on Windows.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 23

[Mike Schroepfer]

Comment on attachment 319050 [details] [diff] [review]
last one, I hope

Great - let's get this in..

Comment 24

[Brian Crowder]

gfxWindowsFonts.cpp: 1.204

Comment 25

[Mats Palmgren (inactive)]

The testcase in bug 430127 crashes with an expected OOM abort, so not very useful
as a crash test.