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)
:
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 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 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 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 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 Robert O'Callahan (:roc) (Exited; 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 Jonathan Kew (:jfkthame) 2011-10-25 13:52:31 PDT
> You actually want moz_xmalloc/xcalloc/xrealloc.

Duh. Yes, of course.
Comment 6 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 Ed Morley [:emorley] 2011-12-10 20:21:33 PST
https://hg.mozilla.org/mozilla-central/rev/17787cebcac4
Comment 9 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 Ed Morley [:emorley] 2011-12-23 19:02:41 PST
https://hg.mozilla.org/mozilla-central/rev/2f0774cdabf4
Comment 12 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 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.