543502 - Possible out of range read in Windows font handling [@ gfxFontFamily::CheckForSimpleFamily()]

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Julian Seward [:jseward]]

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

[Robert Sayre]

Purify should be able to spot this, I would think.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Might mAvailableFonts.Length() be 0?

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: patch v1: check for zero-length mAvailableFonts array

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.

Comment 4

[John Daggett (:jtd)]

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.

Comment 5

[Jonathan Kew [:jfkthame]]

(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.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch v2: check for zero-length mAvailableFonts array; also cull bad families from the font list

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 8

[Julian Seward [:jseward]]

(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

[Brendan Eich [:brendan]]

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

Comment 10

[Jonathan Kew [:jfkthame]]

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.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: followup: replace assertion with (conditional) logging

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.

Comment 12

[Julian Seward [:jseward]]

(In reply to comment #11)
> Created an attachment (id=426719) [details]
> followup: replace assertion with (conditional) logging

Works for me; +1 for it.

Comment 13

[Julian Seward [:jseward]]

Any possibility that the followup patch in Comment #11 can be r?d/landed?

Comment 14

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/8566a4b64fe2