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

RESOLVED FIXED in mozilla11

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Graphite uses malloc to allocate memory it needs; make sure it'll crash cleanly if we run out.
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
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.
Attachment #564775 - Attachment is obsolete: true
Attachment #566480 - Flags: review?(jdaggett)

Comment 3

6 years ago
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.
Attachment #566480 - Flags: review?(jdaggett) → review?(roc)
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.
(Assignee)

Comment 5

6 years ago
> You actually want moz_xmalloc/xcalloc/xrealloc.

Duh. Yes, of course.
(Assignee)

Comment 6

6 years ago
Created attachment 569492 [details] [diff] [review]
patch, make Graphite memory allocation go through mozalloc (corrected)
Attachment #566480 - Attachment is obsolete: true
Attachment #566480 - Flags: review?(roc)
Attachment #569492 - Flags: review?(roc)
Attachment #569492 - Flags: review?(roc) → review+
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/17787cebcac4
Target Milestone: --- → mozilla11

Comment 8

6 years ago
https://hg.mozilla.org/mozilla-central/rev/17787cebcac4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

6 years ago
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.
Attachment #583625 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #583625 - Flags: review?(roc) → review+
(Assignee)

Comment 10

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f0774cdabf4
https://hg.mozilla.org/mozilla-central/rev/2f0774cdabf4
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla11 → mozilla12
(Assignee)

Comment 12

6 years ago
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.
Attachment #583625 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #583625 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 13

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/887afe7f07a4
Target Milestone: mozilla12 → mozilla11
You need to log in before you can comment on or make changes to this bug.