reftest graphite-03a causes spurious "out of memory" crash due to zero-sized allocation request

RESOLVED FIXED in Firefox 11

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jtd, Assigned: jfkthame)

Tracking

Trunk
mozilla13
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11+ fixed, firefox12+ fixed, firefox13 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
graphite-03a stacktrace on winxp:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jdaggett@mozilla.com-ea2710e4d8f8/try-win32-debug/try_xp-debug_test-reftest-build2820.txt.gz

 0  mozalloc.dll!mozalloc_abort(char const * const) [mozalloc_abort.cpp:ea2710e4d8f8 : 79 + 0x0]
 1  mozalloc.dll!mozalloc_handle_oom(unsigned int) [mozalloc_oom.cpp:ea2710e4d8f8 : 60 + 0x9]
 2  mozalloc.dll!moz_xrealloc [mozalloc.cpp:ea2710e4d8f8 : 137 + 0x7]
 3  xul.dll!graphite2::vm::Machine::Code::Code(bool,unsigned char const *,unsigned char const * const,unsigned char,unsigned short,graphite2::Silf const &,graphite2::Face const &) [Code.cpp:ea2710e4d8f8 : 210 + 0x9]
 4  xul.dll!graphite2::Pass::readRules(unsigned char const *,unsigned int,unsigned char const *,unsigned short const *,unsigned short const *,unsigned char const *,unsigned short const *,unsigned char const *,graphite2::Face const &) [Pass.cpp:ea2710e4d8f8 : 192 + 0x28]
 5  xul.dll!graphite2::Pass::readPass(void *,unsigned int,unsigned int,graphite2::Face const &) [Pass.cpp:ea2710e4d8f8 : 152 + 0x1f]
 6  xul.dll!graphite2::Silf::readGraphite(void const *,unsigned int,graphite2::Face const &,unsigned int) [Silf.cpp:ea2710e4d8f8 : 177 + 0x1a]
 7  xul.dll!graphite2::Face::readGraphite() [Face.cpp:ea2710e4d8f8 : 107 + 0x17]
 8  xul.dll!gr_make_face [gr_face.cpp:ea2710e4d8f8 : 60 + 0x8]
 9  xul.dll!gfxGraphiteShaper::ShapeWord(gfxContext *,gfxShapedWord *,wchar_t const *) [gfxGraphiteShaper.cpp:ea2710e4d8f8 : 174 + 0xe]
10  xul.dll!gfxGDIFont::ShapeWord(gfxContext *,gfxShapedWord *,wchar_t const *,bool) [gfxGDIFont.cpp:ea2710e4d8f8 : 166 + 0x1a]
11  xul.dll!gfxFont::GetShapedWord<wchar_t>(gfxContext *,wchar_t const *,unsigned int,unsigned int,int,int,unsigned int) [gfxFont.cpp:ea2710e4d8f8 : 1910 + 0xd]
12  xul.dll!gfxFont::SplitAndInitTextRun<wchar_t>(gfxContext *,gfxTextRun *,wchar_t const *,unsigned int,unsigned int,int) [gfxFont.cpp:ea2710e4d8f8 : 2103 + 0x19]
13  xul.dll!gfxFontGroup::InitScriptRun<wchar_t>(gfxContext *,gfxTextRun *,wchar_t const *,unsigned int,unsigned int,int) [gfxFont.cpp:ea2710e4d8f8 : 3205 + 0x14]
14  xul.dll!gfxFontGroup::InitTextRun<wchar_t>(gfxContext *,gfxTextRun *,wchar_t const *,unsigned int) [gfxFont.cpp:ea2710e4d8f8 : 3151 + 0x2a]

Not sure why an OOM would occur here...
(Assignee)

Comment 1

6 years ago
Strange, those tests all passed tryserver successfully when they were originally written. I wonder what's broken in the meantime.... will try to investigate.
(Assignee)

Updated

6 years ago
Blocks: 700022
(Assignee)

Comment 2

6 years ago
Created attachment 593826 [details] [diff] [review]
patch, don't treat a NULL malloc result as OOM when the requested allocation is zero-sized

So this is actually caused by a problem in the moz_x*alloc wrappers: they trigger an OOM abort when the pointer returned by the underlying *alloc function is NULL, but in the case of a zero-sized allocation request, NULL is in fact a legitimate return value.

The particular case here occurs because during loading of the Burmese font, graphite tries to resize a block to 0 bytes, and on Windows and Linux (but not OS X, apparently) that results in a NULL return from realloc(). The same issue applies to the malloc and calloc wrappers, however.
Assignee: nobody → jfkthame
Attachment #593826 - Flags: review?(jones.chris.g)
(Assignee)

Updated

6 years ago
Summary: reftest graphite-03a causes crash → reftest graphite-03a causes spurious "out of memory" crash due to zero-sized allocation request
Comment on attachment 593826 [details] [diff] [review]
patch, don't treat a NULL malloc result as OOM when the requested allocation is zero-sized

Nice fix.

Is it possible to add a crashtest that uses this font?
Attachment #593826 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 4

6 years ago
The reftest we're intending to add in bug 700022 uses it, so it will in effect be serving as a crashtest as well - is that good enough, or do you want a separate copy labelled as a crashtest as well?
(Assignee)

Comment 5

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0110db9290d
Target Milestone: --- → mozilla13

Comment 6

6 years ago
https://hg.mozilla.org/mozilla-central/rev/d0110db9290d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 732316
Bug 680556 exposed this bug to external XPCOM components, so we either need to back that out Gecko 11 or take this fix there.
Blocks: 680556
tracking-firefox11: --- → ?
tracking-firefox12: --- → ?
Comment on attachment 593826 [details] [diff] [review]
patch, don't treat a NULL malloc result as OOM when the requested allocation is zero-sized

[Approval Request Comment]
Regression caused by (bug #): 680556
User impact if declined: Addons may crash
Testing completed (on m-c, etc.): It's been on m-c for some time, and is very simple.
Risk to taking this patch (and alternatives if risky): Backing out Bug 680556
String changes made by this patch:

This is simple enough to take even at this point in the beta cycle, IMO, but we could always back out Bug 680556.
Attachment #593826 - Flags: approval-mozilla-beta?
Attachment #593826 - Flags: approval-mozilla-aurora?

Updated

5 years ago
tracking-firefox11: ? → +
tracking-firefox12: ? → +
Comment on attachment 593826 [details] [diff] [review]
patch, don't treat a NULL malloc result as OOM when the requested allocation is zero-sized

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #9)
> This is simple enough to take even at this point in the beta cycle, IMO, but
> we could always back out Bug 680556.

Bug 680556 is also causing the OOM crasher in bug 716594. Would it be possible to prepare a backout for Beta 11 instead?

Approving the patch for Aurora 12, but I'd be more comfortable taking a backout of bug 680556 at this point.
Attachment #593826 - Flags: approval-mozilla-beta?
Attachment #593826 - Flags: approval-mozilla-beta-
Attachment #593826 - Flags: approval-mozilla-aurora?
Attachment #593826 - Flags: approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/67ee19b77fe0
status-firefox12: --- → fixed
status-firefox13: --- → fixed
http://hg.mozilla.org/releases/mozilla-beta/rev/b091fc697c22

Updated

5 years ago
status-firefox11: --- → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.