97970 - nsExternalHelperAppService::AddMimeInfoToCache passing null? to nsCStringKey ctor [@ nsCStringKey::nsCStringKey]

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

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

There were 4 crashes in talkback recently (three submitted by the same person)
with the stack:

nsCStringKey::nsCStringKey()
nsExternalHelperAppService::AddMimeInfoToCache()
nsOSHelperAppService::GetFromMIMEType()
nsExternalHelperAppService::DoContent()
nsDocumentOpenInfo::DispatchContent()
nsDocumentOpenInfo::OnStartRequest()
nsHttpChannel::ProcessNormal()
nsHttpChannel::ProcessResponse()
nsHttpChannel::OnStartRequest()
nsOnStartRequestEvent::HandleEvent()
nsARequestObserverEvent::HandlePLEvent()
PL_HandleEvent()
PL_ProcessPendingEvents()
nsEventQueueImpl::ProcessPendingEvents()
event_processor_callback()
our_gdk_io_invoke()
libglib-1.2.so.0 + 0xec10 (0x40342c10)
libglib-1.2.so.0 + 0x102d9 (0x403442d9)
libglib-1.2.so.0 + 0x108e3 (0x403448e3)
libglib-1.2.so.0 + 0x10a7c (0x40344a7c)
libgtk-1.2.so.0 + 0x8dd97 (0x40266d97)
nsAppShell::Run()
nsAppShellService::Run()
main1()
main()

User comments were:
  mouse 1 click on some *.rpm file
  Mozilla seems to crash every time one tries to open a file with extension .asf
even if it is empty.

Might there need to be some check that you're not passing null to the
nsCStringKey constructor, at some level?

(Not marking as topcrash because I don't think 4 reports make it one, although
with the number of people using talkback with nightlys lately and the stability
of those builds, it actually shows up.)

Comment 1

[R.K.Aa.]

see bug 97960

Comment 2

[Matthias Versen [:Matti]]

*** Bug 97137 has been marked as a duplicate of this bug. ***

Comment 3

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

Might it make sense to dupe the other way around?  Oh well...

Comment 4

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

Adding topcrash keyword -- this has been pretty consistent (8 reports in the
past 10 days), and is the #3 clearly identifiable topcrash on linux (although if
all the libc.so.6 crashes or libxpcom.so crashes are the same thing, it's not
really #3).

Comment 5

[greer]

Adding the signature to the summary and cc'ing those lovely talkback people.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK. Here's what seems to be happening. There are actually 2 crashes here, I
think....

If there is an entry in a mailcap file for a mime type (so there is a helper for
it) but no entry in a mime.types file or no extensions listed in such an entry,
then the Unix mailcap/mime.types code was creating an nsIMIMEInfo with an empty
extension list.  Notice that such a beast is nonsensical on Windows, where there
can be no helper if there is no extension.

Now, this is what happened next. 
nsExternalHelperAppService::AddMimeInfoToCache() called
nsMIMEInfoImpl::GetFileExtensions() which proceded to return NS_OK without
setting the extension count out var to 0 if there were no extensions.  Since
AddMimeInfoToCache() did not initialize |count| before calling
GetFileExtensions(), |count| had some bogus value and we proceeded looping
through an empty array, trying to assign its nonexistent entries into a
nsCStringKey variable.  This crashed.  I could reproduce the crash with a simple
install of realplayer...

Now the second case is an empty mimetype.  I can't think of a good reason this
would ever happen if GetFromMIMEType is being called. However, when it does
AddMimeInfoToCache() does not check the return value of GetMIMEType() and can
once again crash, since GetMIMEType() will not initialize the pointer if the
type string has length 0.

Attaching a patch that should fix both crashes (and should fix bug 69864 as
well).  Reviews?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

ccing valeski who wrote nsMIMEInfoImpl::GetFileExtensions().

mscott, valeski, what do you think of the patch?

Comment 9

[Scott MacGregor]

Comment on attachment 49716 [details] [diff] [review]
Proposed patch

sr=mscott

(In the future if you need a sr from me please send me email instead of cc'ing me on the bug per the reviewers guidelines. Thanks!)

Comment 10

[Boris Zbarsky [:bzbarsky]]

checked in on trunk.  nominating for branch

Comment 11

[Keyser Sose]

*** Bug 100232 has been marked as a duplicate of this bug. ***

Comment 12

[Cindy Roberts]

When this has reviews completed, please mark nsbranch+ and write to
[e-mail].  We'd like to get this on the PDT radar for Netscape branch
checkin

Comment 13

[Jaime Rodriguez, Jr.]

Thanks for the fix, can you put it on the branch - PDT+

Comment 14

[Brian Ryner (not reading)]

checked in on the branch.

Comment 15

[Jay Patel [:jay]]

The latest Talkback data shows this last crashed with build 2001092408 on the
MozillaTrunk, we'll have to wait a little while to verify the fix on the branch. 

Tom: Check in a few days to see what the Talkback data looks like for this crash
on the N620 branch.

Comment 16

[benc]

note: 97960 does not seem to be related in any way.

unlike the zero-byte mime.types file crashers, this bug looks like it is XP
apps. I've asked boris for clarifcation...

Comment 17

[benc]

greer: helped verify the other bug w/ crash data, hopefully he will mark this
fixed as well.

No response from bz about this, but after thinking about this last night:

I don't even know how to set up a mime type and NULL the extension. So assuming
I deserve to have this job, it is probably something someone else has been
testing...

so I'm sending this to XP apps, which is actually where most of the dupes for
this bug live.

Comment 18

[sairuh (rarely reading bugmail)]

hrm, i'm not sure how to test/verify this one either. back to benc to see if he
knows of another person who might, and adding qawanted.

Comment 19

[greer]

I'm finding no incidents on the N620 branch with this signature. Marking 
Verified Fixed per Talkback data and comments from jpatel on 10-02.

Comment 20

[benc]

(sarah: I fixed the problem for you. :) <Thanks greer!>)