Last Comment Bug 551661 - unchecked allocations - potential buffer overrun in windows font code
: unchecked allocations - potential buffer overrun in windows font code
Status: RESOLVED FIXED
[sg:critical? (possibly)] [qa-noactio...
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 1.9.2 Branch
: x86 Windows 7
: -- normal (vote)
: ---
Assigned To: Jonathan Kew (:jfkthame)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-11 03:23 PST by Jonathan Kew (:jfkthame)
Modified: 2010-06-22 19:59 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
needed
.4-fixed
needed
.10-fixed


Attachments
check array SetLength() calls for allocation failure (4.73 KB, patch)
2010-03-11 04:28 PST, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
dveditz: approval1.9.2.4+
dveditz: approval1.9.1.10+
Details | Diff | Splinter Review

Description Jonathan Kew (:jfkthame) 2010-03-11 03:23:09 PST
The code for Uniscribe-based text shaping in gfxWindowsFonts.cpp has a number of unchecked uses of nsTArray<T>::SetLength() to resize buffers that will then be passed to Uniscribe APIs. If any of these SetLength() calls fail due to lack of memory (unlikely, but conceivable especially with large textruns), we'd be passing invalid buffers to Uniscribe.

This is being fixed on trunk as part of bug 502906, but that (more extensive) patch is not intended for the 1.9.2 branch.
Comment 1 Jonathan Kew (:jfkthame) 2010-03-11 04:28:57 PST
Created attachment 431835 [details] [diff] [review]
check array SetLength() calls for allocation failure

This should prevent us passing invalid buffers to Uniscribe.
Comment 2 John Daggett (:jtd) 2010-03-15 01:14:12 PDT
Comment on attachment 431835 [details] [diff] [review]
check array SetLength() calls for allocation failure

Looks good.
Comment 3 Jonathan Kew (:jfkthame) 2010-03-15 04:32:39 PDT
Comment on attachment 431835 [details] [diff] [review]
check array SetLength() calls for allocation failure

Requesting approval to land this for 1.9.2.3 - although I don't know of a clear "smoking gun", we have seen issues with pathological pages/scripts that create huge textruns in the past. Allocating several potentially-large arrays for glyph data without checking for failure, and then passing them to Uniscribe, seems like a risk we should not be taking.

So I think we want this on 1.9.2 (and earlier branches?), but I didn't see a "wanted1.9.2?" flag. Should it be blocking? That seems rather extreme for something that is mainly a precautionary measure.
Comment 4 Daniel Veditz [:dveditz] 2010-03-29 10:17:59 PDT
Comment on attachment 431835 [details] [diff] [review]
check array SetLength() calls for allocation failure

Approved for 1.9.2.3, a=dveditz for release-drivers

It looks like this is needed on the 1.9.1 branch. Will this patch work?
Comment 5 Jonathan Kew (:jfkthame) 2010-03-29 14:46:31 PDT
Pushed to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/95c0d61183a1
Comment 6 Jonathan Kew (:jfkthame) 2010-03-29 14:48:21 PDT
Comment on attachment 431835 [details] [diff] [review]
check array SetLength() calls for allocation failure

The same patch applies to 1.9.1 also.
Comment 7 Daniel Veditz [:dveditz] 2010-03-29 16:05:31 PDT
Comment on attachment 431835 [details] [diff] [review]
check array SetLength() calls for allocation failure

Approved for 1.9.1.10, a=dveditz

A checking comment along the lines of a simple "unchecked allocations" would have been better than adding the "potential overflow" part.
Comment 8 Jonathan Kew (:jfkthame) 2010-03-29 16:10:37 PDT
(In reply to comment #7)
> A checking comment along the lines of a simple "unchecked allocations" would
> have been better than adding the "potential overflow" part.

Yeah, I thought about that right after I pushed it! Sorry. :(
I've already modified the comment in my 1.9.1 tree.
Comment 9 Jonathan Kew (:jfkthame) 2010-03-29 16:21:05 PDT
Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1f3da3f9f51e
Comment 10 Al Billings [:abillings] 2010-04-07 16:49:19 PDT
This doesn't seem to be a scenario that we can exploit on demand. Is there a way for QA to verify this bug as fixed for 1.9.2 or 1.9.1?
Comment 11 John Daggett (:jtd) 2010-04-07 17:39:28 PDT
It's basically an out-of-memory scenario, so you'd need to make a testcase that fills memory completely, frees a tidbit, then creates enormous text runs.  In short, difficult to hit these cases but not impossible.

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