Crash [@ gfxMixedFontFamily::ReplaceFontEntry(gfxFontEntry*, gfxFontEntry*) ]

VERIFIED FIXED in Firefox 7

Status

()

Core
Graphics
--
critical
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: bc, Assigned: jfkthame)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla8
x86
Windows XP
crash, regression, reproducible, verified-aurora, verified-beta
Points:
---

Firefox Tracking Flags

(firefox5- wontfix, firefox6- wontfix, firefox7+ fixed, firefox8+ fixed, firefox9+ fixed, status1.9.2 .23-fixed)

Details

(Whiteboard: [sg:critical?][qa-], crash signature, URL)

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
1. http://www.eastoncycling.com/en-us/mountain/products/bars/all-mountain/ea70-mb-281 in Nightly on Windows XP
2. Crash

Ted's exploitable breakpad anaylyser gives this a high exploitability which actually matches the use of deleted memory.

+		this	0x097fc460 {mHdr=0xdddddddd }	const nsTArray_base<nsTArrayDefaultAllocator> * const

  // @return The number of elements in the array.
  size_type Length() const {
=>    return mHdr->mLength;
  }

>	xul.dll!nsTArray_base<nsTArrayDefaultAllocator>::Length()  Line 140 + 0x5 bytes	C++
 	xul.dll!gfxMixedFontFamily::ReplaceFontEntry(gfxFontEntry * aOldFontEntry=0x097fc028, gfxFontEntry * aNewFontEntry=0x097f60c8)  Line 116 + 0xb bytes	C++
 	xul.dll!nsUserFontSet::ReplaceFontEntry(gfxProxyFontEntry * aProxy=0x097fc028, gfxFontEntry * aFontEntry=0x097f60c8)  Line 688	C++
 	xul.dll!gfxUserFontSet::OnLoadComplete(gfxProxyFontEntry * aProxy=0x097fc028, const unsigned char * aFontData=0x00000000, unsigned int aLength=21812, unsigned int aDownloadStatus=0)  Line 560	C++
 	xul.dll!nsFontFaceLoader::OnStreamComplete(nsIStreamLoader * aLoader=0x098ae110, nsISupports * aContext=0x00000000, unsigned int aStatus=0, unsigned int aStringLen=21812, const unsigned char * aString=0x0a0c6c00)  Line 220 + 0x20 bytes	C++
 	xul.dll!nsStreamLoader::OnStopRequest(nsIRequest * request=0x098ad8cc, nsISupports * ctxt=0x00000000, unsigned int aStatus=0)  Line 125 + 0x3e bytes	C++
 	xul.dll!nsCORSListenerProxy::OnStopRequest(nsIRequest * aRequest=0x098ad8cc, nsISupports * aContext=0x00000000, unsigned int aStatusCode=0)  Line 623	C++

I was able to reproduce the crash but not the deleted memory in debug Aurora.
I was not able to reproduce the crash in debug Beta.
I was not able to reproduce with nightly Nightly or Aurora builds.

This was found from crash automation from bp-5279a5dd-12aa-4dcc-bb1d-471152110616 which matches the detected crash.
What about a debug nightly, or MallocScribble on mac?
Whiteboard: [sg:critical?]
(Reporter)

Comment 2

6 years ago
Yes, the stack in comment 0 is from a debug windows Nightly build. I tried today's debug Nightly mac build with scribble enabled and still did not crash. I just reproduced the crash with today's debug Nightly on windows xp with the same deleted memory in mHdr. I have it in visual studio at the moment and will leave it there for a bit if you need me to be a debugging bot.

Note when I say nightly I mean an opt build downloaded from ftp.m.o regardless of branch. When I say Nightly (cap N) it is a trunk build.
Maybe something specific to WinXP fonts? Or specifically the fonts installed on your test system?
John, can you look into this?
Assignee: nobody → jdaggett
status-firefox5: --- → wontfix
status-firefox6: --- → affected
status-firefox7: --- → affected
tracking-firefox5: --- → -
tracking-firefox6: --- → +
tracking-firefox7: --- → +
(Reporter)

Comment 5

6 years ago
Maybe just XP. I don't see it in Windows 7 with the automation. These are pretty generic vms.

Comment 6

6 years ago
(In reply to comment #4)
> John, can you look into this?

I'm flying today, I won't be able to look at this until next week.  The location of the crash indicates a downloadable font, it's not specific to fonts on XP.  But I think this is something jkew already fixed, maybe it just needs to get pushed to a difference branch?  Jonathan, can you comment?
Jonathan, sounds like this needs attention from you, reassigning.
Assignee: jdaggett → jfkthame
(Assignee)

Comment 8

6 years ago
The recent fixes I'm aware of in this area are bug 655138 and bug 650639, but both of those are present in Aurora as well as trunk code at this point.

Crash-stats indicates this is still happening on aurora (6.0a2) as well as trunk, plus a variety of earlier versions (4.0.1, even 3.x). Interestingly, the most recent trunk build that shows up is from 06/24 (a week ago, at this point), but I don't see anything in the pushlog from around that time that jumps out as a likely explanation for why the crashes might have stopped.

Also, crash-stats doesn't list any occurrences on FF5.0. I don't have any explanation for that, when it's happening in both 4.0.1 and 6.0a2.

My guess is that the crashes occur when the gfxFontFamily associated with a proxy font entry has been deleted prior to the font load completing, but I haven't figured out an exact scenario that would lead to this - let alone managed to reproduce it. Although we've made some changes in this area lately (especially bug 633299 and followup fixes), I don't think that accounts for the problem here, as we see this crash signature even in much older releases.
(Assignee)

Comment 9

6 years ago
Created attachment 543614 [details] [diff] [review]
possible band-aid

I have been running a current Aurora build on Windows for a while hoping to reproduce this, but haven't yet had any success. However, something we could try as a possible band-aid would be to make the nsFontFaceLoader hold a reference to the font family, as well as to the actual proxy on whose behalf it is loading. (Note that the proxy font entry does *not* have a strong reference to its family, only a weak pointer.) If there's some race condition or similar whereby the family can get deleted right before the load completes, this should ensure that it remains valid long enough to avoid this crash.

Unless you have any better ideas, John, I'd suggest we try landing this - I don't see how it could do any harm, at least - and see whether it has any effect on crash-stats.
Attachment #543614 - Flags: review?(jdaggett)
Seems unlikely that we'll take this for 6, or that we'll even know if it's fixed or not. Tracking for newer versions though.
status-firefox6: affected → wontfix
status-firefox8: --- → affected
tracking-firefox6: + → -
tracking-firefox8: --- → +
John, can you review here? We'd love to get this band-aid bug in to see if it helps, which based on what Marcia tells me we'd know whether it helped or not in a matter of days.
(Reporter)

Comment 12

6 years ago
fwiw, I am still seeing related crashes in automation and could retest within a matter of hours once this lands.

Updated

6 years ago
Attachment #543614 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 13

6 years ago
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/89842a2a40c6

Let's keep this bug open after this merges to m-c, at least until we have time to see whether this affects the crashiness at all.
Whiteboard: [sg:critical?] → [sg:critical?] [inbound]
http://hg.mozilla.org/mozilla-central/rev/89842a2a40c6
Whiteboard: [sg:critical?] [inbound] → [sg:critical?]
Target Milestone: --- → mozilla8
bc: please retest and see if the band-aide helped any.

Updated

6 years ago
status-firefox9: --- → affected
tracking-firefox9: --- → +
I don't see any crashes in 8.0 builds after the Aug 9 checkin. There were only a handful before, but enough in 8 and 7 that I feel pretty good that we've got a real fix here.
Comment on attachment 543614 [details] [diff] [review]
possible band-aid

This patch seems to have fixed the security hole in Firefox 8. It's safe and should be landed on Firefox 7 beta.
Attachment #543614 - Flags: approval-mozilla-beta?
(Reporter)

Comment 18

6 years ago
I only see this on beta now. Appeared fixed (papered over?) on Nightly and Aurora. It is already on Aurora isn't it? http://hg.mozilla.org/releases/mozilla-aurora/rev/89842a2a40c6

Updated

6 years ago
Duplicate of this bug: 680475
This is in the top fifty crashes for Firefox 6; the band-aid should IMO be considered if we do a patch for that release.  And it should definitely be landed on beta ASAP.

https://crash-stats.mozilla.com/report/list?range_value=7&range_unit=days&date=2011-08-18&signature=gfxMixedFontFamily%3A%3AReplaceFontEntry%28gfxFontEntry*%2C%20gfxFontEntry*%29&version=Firefox%3A6.0

Updated

6 years ago
status-firefox6: wontfix → affected
tracking-firefox6: - → ?
Comment on attachment 543614 [details] [diff] [review]
possible band-aid

Approved for beta per today's triage meeting.
Attachment #543614 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 22

6 years ago
yeah, lets get this fixed quickly and in as many releases as possible.

signature has jumped to over 180 crashes per day after the release of 6.0

         gfxMixedFontFamily::ReplaceFontEntry.gfxFontEntry...gfxFontEntry..
date     total    breakdown by build
         crashes  count build, count build, ...

20110818 187  	174 6.02011081116, 
        	4 7.0a22011081604, 	3 3.5.12009071509, 
        	2 7.02011081615, 	2 6.02011080403, 
        	1 8.0a12011072003, 	1 3.6.132010120307, 

and is visible on wide variety of sites.

domains of sites
  16 file://
  13 \N//
   5 http://www.ibahia.com
   4 http://www.probikekit.com
   4 http://www.blogger.com
   3 javascript:''//
   3 http://www.xcid.net
   3 http://www.lggetconnected.com
   3 http://www.eastoncycling.com
   2 http://www.conceptclub.ru
   2 http://veldev.com
   2 http://tracking.traviangames.com
   2 http://swetozar-online.ru
   2 http://ilumalite.syntaxmarketing.com.au
   2 http://fr.novopress.info
   2 http://feedproxy.google.com
   2 http://demos.foxymoron.co.in
   2 http://conceptclub.ru
   2 http://192.168.0.100
   2 http://178.79.144.232
   1 https://www.change.org
   1 https://plus.google.com
   1 https://opdagelse.telenor.dk
   1 https://ib.nab.com.au
   1 https://docs.google.com
   1 http://yfrog.com
   1 http://www.youtube.com
   1 http://www.yellowpagesmyanmar.net

urls for testing
  13 \N
   3 javascript:''
   2 http://www.xcid.net/contacto/survey.php?action=manage
   2 http://swetozar-online.ru/catalog/result/
   2 http://demos.foxymoron.co.in/lastwilderness/
   2 http://192.168.0.100/sliced/inne/Crimson%20Oak/DW%20Studio/blog.html
   2 file:///D:/portfolio/index.html
   1 https://www.change.org/login
   1 https://plus.google.com/u/0/_/notifications/frame?hl=en&origin=https%3A%2F%2Fmail.google.com&jsh=r%3Bgc%2F22954852-97da52d8#pid=23&id=gbsf&parent=https%3A%2F%2Fmail.google.com&rpctoken=82175096&_methods=onError%2ConInfo%2ChideNotificationWidget%2CpostShar
   1 https://opdagelse.telenor.dk/Bliv-underholdt/Musik/Fa-WiMP/Aktiver-WiMP/
   1 https://ib.nab.com.au/nabib/payments_transferList.ctl
   1 https://docs.google.com/a/captiway.com/?tab=oo#home
   1 http://yfrog.com/h6athizj
   1 http://www.youtube.com/watch?v=HyQqZh2Yx8I&feature=mr_meh&list=ULFHp2KgyQUFk&index=21&playnext=0
   1 http://www.yellowpagesmyanmar.net/list_bycat.php?page=2&cid=133&hcid=&loc=1
   1 http://www.xcid.net/
   1 http://www.web-web-11.info/unicalle//index.php?option=com_diplomas&view=diploma1&Itemid=8
   1 http://www.vodafonefreezone.com/Article.aspx?PageID=108&PageName=senle-ben-paketleri
   1 http://www.turkcell.de/internet
   1 http://www.thenorthface.cl/catalogo/hombres/footwear/hiking/2016_m-syncline-gtx
   1 http://www.thedeyjaycollection.com/shop/
   1 http://www.theatre-octogone.ch/11/index.php?page=saison-theatre
   1 http://www.sputnik-centar.hr/
   1 http://www.sounddepo.com/2011/
   1 http://www.silent-guilde.fr/
   1 http://www.sanitarium.com.au/products/vegetarian
   1 http://www.ray-ban.com/germany/store-locator
(Assignee)

Comment 23

6 years ago
Pushed to mozilla-beta:
http://hg.mozilla.org/releases/mozilla-beta/rev/b8d41ea97ce8
(Reporter)

Comment 24

6 years ago
beta does not appear to crash on my previous list of urls. This should be FIXED and the stati set to fixed, right?
(Assignee)

Comment 25

6 years ago
If you were able to reproduce it consistently before, and can confirm it's fixed, that's great; please mark accordingly. I was never able to reproduce the crash locally, so intended to wait and see what crash-stats showed...
Ok, marking this FIXED then!
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox6: affected → wontfix
status-firefox7: affected → fixed
status-firefox8: affected → fixed
status-firefox9: affected → fixed
tracking-firefox6: ? → -
Resolution: --- → FIXED

Comment 27

6 years ago
Does this affect 1.9.2? It looks like it does as the crash signature shows up on 3.6.x. If so we'll need to take it for 1.9.2.21
(Assignee)

Comment 28

6 years ago
(In reply to Christian Legnitto [:LegNeato] from comment #27)
> Does this affect 1.9.2?

It almost certainly does. The 1.9.2 crash reports look as though they're coming from the same code path. We've never really explained the underlying cause, but the precautionary patch here seems to have stopped the crashes, as far as I can tell.

The patch needs minor adjustment to apply on 1.9.2; I'm just building locally to check it's OK, then will post it here.
(Assignee)

Comment 29

6 years ago
Created attachment 556625 [details] [diff] [review]
patch for 1.9.2 branch
Attachment #556625 - Flags: approval1.9.2.21?

Comment 30

6 years ago
Comment on attachment 556625 [details] [diff] [review]
patch for 1.9.2 branch

Thanks! Approved for releases/mozilla-1.9.2. Please land on the default branch asap
Attachment #556625 - Flags: approval1.9.2.23? → approval1.9.2.23+
(Assignee)

Comment 31

6 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f5dd7005daa8
status1.9.2: --- → .23-fixed
qa-, Bob can you please verify this is fixed? Thanks
Whiteboard: [sg:critical?] → [sg:critical?][qa-]
(Reporter)

Comment 33

6 years ago
ashughes: i retested my urls and did not see the crash on 1.9.2, beta/7, aurora/8, nightly/9.
Thanks Bob. I'm calling this verified.
Status: RESOLVED → VERIFIED
Keywords: verified-aurora, verified-beta
Group: core-security
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.