Closed Bug 691505 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(2 files, 2 obsolete files)

Graphite uses malloc to allocate memory it needs; make sure it'll crash cleanly if we run out.
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.
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 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.
> You actually want moz_xmalloc/xcalloc/xrealloc.

Duh. Yes, of course.
Attachment #566480 - Attachment is obsolete: true
Attachment #566480 - Flags: review?(roc)
Attachment #569492 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/17787cebcac4
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/2f0774cdabf4
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla11 → mozilla12
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?
Attachment #583625 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.