691505 - ensure Graphite uses infallible malloc to give well-defined OOM behavior

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Kew [:jfkthame]]

Graphite uses malloc to allocate memory it needs; make sure it'll crash cleanly if we run out.

Comment 1

[Jonathan Kew [:jfkthame]]

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

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

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) (email my personal email if necessary)]

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

> You actually want moz_xmalloc/xcalloc/xrealloc.

Duh. Yes, of course.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch, make Graphite memory allocation go through mozalloc (corrected)

Comment 7

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17787cebcac4

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/17787cebcac4

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: 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 10

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f0774cdabf4

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2f0774cdabf4

Comment 12

[Jonathan Kew [:jfkthame]]

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/887afe7f07a4