Last Comment Bug 691505 - ensure Graphite uses infallible malloc to give well-defined OOM behavior
: ensure Graphite uses infallible malloc to give well-defined OOM behavior
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Jonathan Kew (:jfkthame)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 631479
  Show dependency treegraph
 
Reported: 2011-10-03 13:55 PDT by Jonathan Kew (:jfkthame)
Modified: 2012-02-01 14:00 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, make Graphite memory allocation go through mozalloc (783 bytes, patch)
2011-10-05 03:09 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
patch, make Graphite memory allocation go through mozalloc (3.24 KB, patch)
2011-10-12 02:36 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
patch, make Graphite memory allocation go through mozalloc (corrected) (3.24 KB, patch)
2011-10-25 13:56 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
followup, use the correct #define name (1001 bytes, patch)
2011-12-21 14:13 PST, Jonathan Kew (:jfkthame)
roc: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Jonathan Kew (:jfkthame) 2011-10-03 13:55:59 PDT
Graphite uses malloc to allocate memory it needs; make sure it'll crash cleanly if we run out.
Comment 1 User image Jonathan Kew (:jfkthame) 2011-10-05 03:09:21 PDT
Created attachment 564775 [details] [diff] [review]
patch, make Graphite memory allocation go through mozalloc

This is a possible patch to ensure we crash safely on OOM during Graphite processing, rather than risking anything exploitable due to unchecked allocations or inconsistent state.

I've written to Martin to see if we can get a solution for this into the upstream codebase rather than needing to patch the Graphite source locally, so we should await a response to that before deciding how to proceed.
Comment 2 User image Jonathan Kew (:jfkthame) 2011-10-12 02:36:02 PDT
Created attachment 566480 [details] [diff] [review]
patch, make Graphite memory allocation go through mozalloc

The graphite code now provides an option to include an extra "custom" header; we can use this to insert our own definitions for malloc() etc.
Comment 3 User image John Daggett (:jtd) 2011-10-25 03:46:33 PDT
Comment on attachment 566480 [details] [diff] [review]
patch, make Graphite memory allocation go through mozalloc

This looks fine but I actually don't know whether mapping to moz_malloc is the right way for this.  Passing along to roc.
Comment 4 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-25 11:53:18 PDT
Comment on attachment 566480 [details] [diff] [review]
patch, make Graphite memory allocation go through mozalloc

Review of attachment 566480 [details] [diff] [review]:
-----------------------------------------------------------------

You actually want moz_xmalloc/xcalloc/xrealloc.
Comment 5 User image Jonathan Kew (:jfkthame) 2011-10-25 13:52:31 PDT
> You actually want moz_xmalloc/xcalloc/xrealloc.

Duh. Yes, of course.
Comment 6 User image Jonathan Kew (:jfkthame) 2011-10-25 13:56:03 PDT
Created attachment 569492 [details] [diff] [review]
patch, make Graphite memory allocation go through mozalloc (corrected)
Comment 8 User image Ed Morley [:emorley] 2011-12-10 20:21:33 PST
https://hg.mozilla.org/mozilla-central/rev/17787cebcac4
Comment 9 User image Jonathan Kew (:jfkthame) 2011-12-21 14:13:25 PST
Created attachment 583625 [details] [diff] [review]
followup, use the correct #define name

Argh, I goofed big-time here: the symbol that's supported in the Graphite code is GR2_CUSTOM_HEADER, not GR_CUSTOM_HEADER, and so our "replacement" of the memory allocation with moz_xmalloc etc is not actually taking effect at present.

The fix, obviously, is trivial. Once this lands on trunk, I'll also nominate it for Aurora, as the Graphite code has just migrated there in the latest uplift.
Comment 11 User image Ed Morley [:emorley] 2011-12-23 19:02:41 PST
https://hg.mozilla.org/mozilla-central/rev/2f0774cdabf4
Comment 12 User image Jonathan Kew (:jfkthame) 2012-01-02 08:51:28 PST
Comment on attachment 583625 [details] [diff] [review]
followup, use the correct #define name

This is a trivial fix to the integration of Graphite font shaping code, which merged to Aurora in December. The intention of this bug was to make graphite use moz_xmalloc for memory allocation, to ensure that we get a safe abort in case of OOM, but due to a typo in the original patch, it is currently using standard malloc instead.

As graphite is preffed off by default, this is only a potential issue for users who deliberately enable it. Nevertheless, we should fix is as a precautionary measure, rather than risk someone finding a way to exploit a poorly-handled OOM situation within graphite.
Comment 13 User image Jonathan Kew (:jfkthame) 2012-01-05 01:51:37 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/887afe7f07a4

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