Last Comment Bug 738197 - handle potential failures in gfxFont::GetShapedWord more robustly
: handle potential failures in gfxFont::GetShapedWord more robustly
[sg:dos oom null deref]
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla14
Assigned To: Jonathan Kew (:jfkthame)
Depends on:
  Show dependency treegraph
Reported: 2012-03-22 04:06 PDT by Jonathan Kew (:jfkthame)
Modified: 2012-03-30 11:31 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch, check calls that could fail, and bail out if necessary (2.21 KB, patch)
2012-03-22 04:06 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review

Description User image Jonathan Kew (:jfkthame) 2012-03-22 04:06:31 PDT
Created attachment 608296 [details] [diff] [review]
patch, check calls that could fail, and bail out if necessary

GetShapedWord makes a few calls that are potentially subject to OOM failures, so we need to be more careful to check the results and bail out if they fail.

We don't want to make these infallible, as that means the browser will crash if over-stressed by huge amounts of text (e.g. exponentially-growing content inserted by a script); it's better to fail "gracefully" by simply not rendering the excessive text.

Marking security-sensitive as a precaution, although in practice I think it'd be pretty hard to come up with an exploit that managed to hit OOM under exactly the right conditions to do something nasty here.
Comment 1 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2012-03-22 09:04:12 PDT
Comment on attachment 608296 [details] [diff] [review]
patch, check calls that could fail, and bail out if necessary

Review of attachment 608296 [details] [diff] [review]:

::: gfx/thebes/gfxFont.cpp
@@ +2080,5 @@
>      CacheHashEntry *entry = mWordCache.PutEntry(key);
> +    if (!entry) {
> +        NS_WARNING("failed to create word cache entry - expect missing text");
> +        return nsnull;

I believe nsTHashtable is actually infallible now? worth checking
Comment 2 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-22 09:07:43 PDT
It's not.
Comment 3 User image Jonathan Kew (:jfkthame) 2012-03-22 09:12:51 PDT
(In reply to Kyle Huey [:khuey] ( from comment #2)
> It's not.

Right; I checked, actually!

(Bug 734847 proposes to make it infallible by default, but I think we'll want to keep this as fallible anyway and handle OOM safely.)
Comment 5 User image Jonathan Kew (:jfkthame) 2012-03-23 15:03:36 PDT
This was backed out, along with other patches from the same push, by:

However, I don't believe this patch was actually the problem there, so I've re-pushed it to inbound:
Comment 6 User image Ed Morley [:emorley] 2012-03-24 13:50:19 PDT

jfkthame, this doesn't have any other tracking flags set at present or sg:foo set in the whiteboard, so not sure if this will be caught by the security team's triage saved search. If you feel this needs backporting or review by them, please add as appropriate :-)
Comment 7 User image Daniel Veditz [:dveditz] 2012-03-30 11:31:33 PDT
Given the patch the bug results in null derefs in each place I checked:
  gfxFcFont::ShapeWord (pango fallback)
  gfxGDIShaper::ShapeWord (assuming ::GetGlyphIndicesW crashes or returns error)

I think I missed a couple but they all did much the same thing.

Note You need to log in before you can comment on or make changes to this bug.