Closed
Bug 543502
Opened 14 years ago
Closed 14 years ago
Possible out of range read in Windows font handling [@ gfxFontFamily::CheckForSimpleFamily()]
Categories
(Core :: Layout: Text and Fonts, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jseward, Assigned: jfkthame)
References
Details
(Keywords: crash, topcrash, Whiteboard: [sg:investigate])
Crash Data
Attachments
(2 files, 1 obsolete file)
2.62 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
1.19 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
Let me begin by saying, this report was obtained by using a very experimental rig running a Win32 build of Fx on Wine on Valgrind. So I am not entirely sure of my ground, and don't want to waste people's time if this turns out to be a red herring. That said .. It appears there is an out of range 16-bit read in font handling during Fx startup on WinXP. The resulting Valgrind error is shown below. This is with mozilla-central of a week back, debug build on Windows (MSVC9, WinXP), carted over to a Linux box, run on wine GIT trunk, running on a slightly modified version of Valgrind trunk. Unfortunately the debug machinery provides source locations for every frame except the one we really need to see :-( Anyway, gfxFont.cpp:327 is this PRInt16 firstStretch = mAvailableFonts[0]->Stretch(); which afaics is a call to this (gfxFont.h:193) PRInt16 Stretch() const { return mStretch; } which appears to be a simple read of gfxFontEntry::mStretch. So I'm confused why V would complain that we are reading just off the end of the object, unless it is not an object of the correct type. Looking at the (alleged!) allocation point for the object, at nsgenericfactory.cpp:189, I am none the wiser what the type of the allocated object is/was intended to be. Invalid read of size 2 at 0x1105399A: gfxFontEntry::Stretch (in /home/sewardj/.wine/drive_c/ MOZ/MC_Incoming/ff-deb/dist/bin/xul.dll) by 0x1105383A: gfxFontFamily::CheckForSimpleFamily (gfxfont.cpp:327) by 0x1106E783: gfxPlatformFontList::RunLoader (gfxplatformfontlist.cpp:475) by 0x11075D0B: gfxFontInfoLoader::LoaderTimerFire (gfxfontutils.h:727) by 0x11075CB1: gfxFontInfoLoader::LoaderTimerCallback (gfxfontutils.h:717) by 0x11007E3D: nsTimerImpl::Fire (nstimerimpl.cpp:427) by 0x11008020: nsTimerEvent::Run (nstimerimpl.cpp:519) by 0x10FF4A09: nsThread:rocessNextEvent (nsthread.cpp:527) by 0x10FA9232: NS_ProcessNextEvent_P (nsthreadutils.cpp:250) by 0x1116077B: mozilla::ipc::MessagePump::Run (messagepump.cpp:142) by 0x10F0F545: MessageLoop::RunInternal (message_loop.cc:211) by 0x10F0F481: MessageLoop::RunHandler (message_loop.cc:194) Address 0x7fc3a59c is 4 bytes after a block of size 56 alloc'd at 0xCD418FE: RtlAllocateHeap (heap.c:254) by 0x7F43103D: _heap_alloc_base (malloc.c:105) by 0x7F43FD75: _heap_alloc_dbg_impl (dbgheap.c:427) by 0x7F43FB2E: _nh_malloc_dbg_impl (dbgheap.c:239) by 0x7F43FADB: _nh_malloc_dbg (dbgheap.c:296) by 0x7F44B25A: malloc (dbgmalloc.c:56) by 0x7F42D690: operator new (new.cpp:59) by 0x10FCF3DD: nsGenericFactory::Create (nsgenericfactory.cpp:189) by 0x10FCF485: NS_NewGenericFactory (nsgenericfactory.cpp:207) by 0x10FED742: RegisterGenericFactory (nsxpcominit.cpp:302) by 0x10FED4F1: NS_InitXPCOM3_P (nsxpcominit.cpp:665) by 0x1000EB9C: ScopedXPCOMStartup::Initialize (nsapprunner.cpp:1099)
Comment 1•14 years ago
|
||
Purify should be able to spot this, I would think.
Updated•14 years ago
|
Whiteboard: [sg:investigate]
Might mAvailableFonts.Length() be 0?
Assignee | ||
Comment 3•14 years ago
|
||
I think the likeliest scenario is that there's a "bad" font family for which GDIFontFamily::FindStyleVariations() is not loading any faces; in this case, CheckForSimpleFamily() would access a non-existent array element. Eventually, I think we should eliminate such families from the font list altogether, but in the meantime we can make this code more robust by checking that the array length is non-zero before accessing the first element.
Assignee: nobody → jfkthame
Attachment #424674 -
Flags: review?(jdaggett)
Comment 4•14 years ago
|
||
Rather than this wouldn't it be better to catch the "no faces" case in RunLoader(), right after familyEntry->FindStyleVariations()? Even better, stick these fonts in a "funky fonts" list and remove them from the various hashtables within FinishLoader.
Assignee | ||
Comment 5•14 years ago
|
||
(In reply to comment #4) > Rather than this wouldn't it be better to catch the "no faces" case in > RunLoader(), right after familyEntry->FindStyleVariations()? > > Even better, stick these fonts in a "funky fonts" list and remove them from the > various hashtables within FinishLoader. That would be fine, but I think we should make this function safe anyway.
Assignee | ||
Comment 6•14 years ago
|
||
I think we can remove bad families from the font list in RunLoader() as in this version of the patch. Note that it is possible for bad families to exist in the font list temporarily, before the loader has finished, so any code that looks at families should be prepared to deal with the possibility that no faces exist. Currently, the only place CheckForSimpleFamily gets called is from the loader, but to reduce the risk of breaking this again sometime in the future I still think we should add the check to CheckForSimpleFamily as a safety measure.
Attachment #424674 -
Attachment is obsolete: true
Attachment #424754 -
Flags: review?(jdaggett)
Attachment #424674 -
Flags: review?(jdaggett)
I think this is showing up in the trunk topcrash list (as "gfxFontFamily::CheckForSimpleFamily()"): http://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=exact&query=gfxFontFamily%3A%3ACheckForSimpleFamily%28%29&date=&range_value=1&range_unit=weeks&process_type=all&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&signature=gfxFontFamily%3A%3ACheckForSimpleFamily%28%29 unless that's somehow a different problem in the same function.
Summary: Possible out of range read in Windows font handling → Possible out of range read in Windows font handling [@ gfxFontFamily::CheckForSimpleFamily()]
Updated•14 years ago
|
Attachment #424754 -
Flags: review?(jdaggett) → review+
Reporter | ||
Comment 8•14 years ago
|
||
(In reply to comment #6) > Created an attachment (id=424754) [details] > patch v2: check for zero-length mAvailableFonts array; also cull bad families > from the font list With this in place, I can no longer trigger the out of range read, so it looks like a successful fix. There is however something that looks like it might be related: with a debug build, both with and without the patch, I get an assertion failure ASSERTION: no styles available in family CMU\ Typewriter\ Text\ Variable\ Wi: 'mAvailableFonts.Length() != 0', file e:/MOZ/WD_MCfeb10/gfx/thebes/src/gfxGDIFontList.cpp, line 511 Should that still happen? It's certainly annoying from the point of view of debugging Fx on Wine on Linux since every Fx startup now pops up this dialogue box on which I have to click "Ignore".
Comment 9•14 years ago
|
||
Assertions should not be tolerated as if they were warnings. Either this one should be a warning (but warnings are generally useless, a waste or worse on most users, unlikely to reach a developer in time), or it should be turned into silent but defensive code if needed, or removed if defensive code follows. /be
Assignee | ||
Comment 10•14 years ago
|
||
pushed crash fix: http://hg.mozilla.org/mozilla-central/rev/b6a6b5a1f048 Leaving open for now to look into the remaining assertion (comments 8 & 9), although this should no longer be a crash risk AFAIK.
Assignee | ||
Comment 11•14 years ago
|
||
I suggest that we can drop the "no faces" assertion in GDIFontFamily::FindStyleVariations(), as the higher-level code now handles this issue better; however, as a diagnostic aid it might still be nice to have the possibility of logging such "flaky families". This patch replaces the assertion with PR_LOG-based logging, so that it will be silent by default but is still available if wanted.
Attachment #426719 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #426719 -
Flags: review? → review?(jdaggett)
Reporter | ||
Comment 12•14 years ago
|
||
(In reply to comment #11) > Created an attachment (id=426719) [details] > followup: replace assertion with (conditional) logging Works for me; +1 for it.
Reporter | ||
Comment 13•14 years ago
|
||
Any possibility that the followup patch in Comment #11 can be r?d/landed?
Updated•14 years ago
|
Attachment #426719 -
Flags: review?(jdaggett) → review+
Assignee | ||
Comment 14•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/8566a4b64fe2
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Crash Signature: [@ gfxFontFamily::CheckForSimpleFamily()]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•