551661 - unchecked allocations - potential buffer overrun in windows font code

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Kew [:jfkthame]]

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]]

Attachment: check array SetLength() calls for allocation failure

This should prevent us passing invalid buffers to Uniscribe.

Comment 2

[John Daggett (:jtd)]

Comment on attachment 431835 [details] [diff] [review]
check array SetLength() calls for allocation failure

Looks good.

Comment 3

[Jonathan Kew [:jfkthame]]

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]]

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]]

Pushed to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/95c0d61183a1

Comment 6

[Jonathan Kew [:jfkthame]]

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]]

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]]

(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]]

Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1f3da3f9f51e

Comment 10

[Al Billings [:abillings - ex-MoCo]]

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)]

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.