540566 - nsCacheEntryDescriptor::GetDeviceID may crash in NS_strdup [@strlen | NS_strdup(char const*) ]

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Makoto Kato [:m_kato]]

If mCacheEntry->GetDeviceID() returns null, Firefox will crash in NS_strdup().

Trunk
http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.7a1pre&query_search=signature&query_type=exact&query=strlen%20|%20NS_strdup%28char%20const*%29&date=&range_value=1&range_unit=weeks&do_query=1&signature=strlen%20|%20NS_strdup%28char%20const*%29

3.6
http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.6&query_search=signature&query_type=exact&query=strlen%20|%20NS_strdup%28char%20const*%29&date=&range_value=1&range_unit=weeks&do_query=1&signature=strlen%20|%20NS_strdup%28char%20const*%29

3.5
http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.5.7&query_search=signature&query_type=exact&query=strlen%20|%20NS_strdup%28char%20const*%29&date=&range_value=1&range_unit=weeks&do_query=1&signature=strlen%20|%20NS_strdup%28char%20const*%29

Comment 1

[timeless]

Signature	strlen | NS_strdup(char const*)
UUID	1f071269-bff9-4637-bed3-9149b2100118
Time 	2010-01-18 12:54:21.453810
Uptime	47677
Last Crash	634943 seconds before submission
Product	Firefox
Version	3.7a1pre
Build ID	20100116042715
Branch	1.9.3
OS	Windows NT
OS Version	6.1.7600
CPU	x86
CPU Info	GenuineIntel family 6 model 15 stepping 11
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x0
User Comments	
Processor Notes 	
Related Bugs

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	mozcrt19.dll 	strlen 	strlen.asm:81
1 	xul.dll 	NS_strdup 	obj-firefox/xpcom/build/nsCRTGlue.cpp:145
2 	xul.dll 	nsCacheEntryDescriptor::GetDeviceID 	netwerk/cache/src/nsCacheEntryDescriptor.cpp:98
3 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102

Comment 2

[timeless]

Attachment: patch

note that afaict this is never called by Gecko c++ code, so it's just used by us in js and others whereever. Given that it has always crashed, I don't think people can have expectations about what it returns in this case :).

Comment 3

[Makoto Kato [:m_kato]]

timeless, you should fix nsCacheEntryInfo::GetDeviceID(), too

http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/src/nsCacheEntry.cpp#302

Comment 4

[timeless]

Attachment: both

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 422340 [details] [diff] [review]
both

- nsCacheEntryInfo is only used for entries that already have a device. Consequently, you don't need this change.

- Please add an xpcshell testcase. Here's a crashing testcase:
var cache = Components.classes["@mozilla.org/network/cache-service;1"].getService(Components.interfaces.nsICacheService);
var session = cache.createSession("client", 0, true);
var entry = session.openCacheEntry("key", 2, true);
entry.deviceID

(but use constants instead of these fixed numbers)

Comment 6

[John J. Barton]

While reproducing a firebug user report, 
http://code.google.com/p/fbug/issues/detail?id=2779
I crashed FF 3.6
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100201 Namoroka/3.6.2pre
with
http://crash-stats.mozilla.com/report/index/7cd94720-d565-4b4a-b357-55e262100201
and the link lead here. Several other comments on the crash-stats page mention Firebug.

I'm confused by comment 5
> Consequently, you don't need this change.

Is this is fix or no?

Comment 7

[timeless]

Attachment: with test

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

John: with what version of Firebug did you crash? You've marked this as a Firebug P1 which is serious, but it would be good to know if it's blocking an upcoming Firebug release, or causes problems with the existing one, and if that's the case, how often it comes up.

Not blocking for now, but if you can answer those questions, please renominate.

Comment 10

[jay]

Sorry, I thought you also had seen my crash report, which includes that info:

http://crash-stats.mozilla.com/report/index/bp-6c1cee4a-250f-42cb-8000-554262100201

It's Firebug version 1.5.0.

And to prove it was Firebug, I deleted all other AddOns when I tested and crahsed.

Comment 11

[John J. Barton]

(In reply to comment #9)
> John: with what version of Firebug did you crash? 

I crashed 1.6a4; the original Firebug user report was against 1.5.0 (the current AMO release).

> You've marked this as a
> Firebug P1 which is serious, but it would be good to know if it's blocking an
> upcoming Firebug release, or causes problems with the existing one, and if
> that's the case, how often it comes up.

I would say this crash is rare, based little information. It must require more that just Firebug + full cache refresh or we would hear more.

> 
> Not blocking for now, but if you can answer those questions, please renominate.

This is a crasher with multiple reports and a patch. In my opinion this should be reviewed and land on 1.9.3 promptly.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 424612 [details] [diff] [review]
with test

+  do_test_finished();

You only need this if you used do_test_pending, which you don't

Also, tests should preferrably be in the public domain, see http://www.mozilla.org/MPL/license-policy.html

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

I'd love to have a JS stack trace of the Firebug crashes...

Does Firebug access the deviceID of cache entries anywhere? If so could you post a link to the code?

Re comment 6, when I said "you don't need this change" I was referring to only half of the patch.

Comment 14

[jay]

Christian,

You said "I'd love to have a JS stack trace of the Firebug crashes...".  If you tell me what you want, I can try and get it for you.  

I just turned all my other AddOn's off, turned Firebug on, and FF consistently crashes again when I navigate to any page.

Jay

Comment 15

[John J. Barton]

(In reply to comment #14)
> 
> I just turned all my other AddOn's off, turned Firebug on, and FF consistently
> crashes again when I navigate to any page.

Jay, there is almost certainly something else wrong on our installation. We have some millions of users and this crash is rare.

please install Firebug in a clean newly created Firefox profile. (See 
http://getfirebug.com/wiki/index.php/FAQ for hints). If you crash then it issue is in your Firefox or OS install; if not then the problem is a setting in your current profile.

Comment 16

[John J. Barton]

On the crash there seems to be just one JS frame, a callback that accesses
 descriptor.deviceID:
http://code.google.com/p/fbug/source/browse/branches/firebug1.6/content/firebug/net.js#4388

Comment 17

[jay]

As John suggested, I created a new profile and the issue has gone away.  For me at least, something must have gotten corrupted locally and impacted Firebug, etc.

Thanks.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #16)
> On the crash there seems to be just one JS frame, a callback that accesses
>  descriptor.deviceID:
> http://code.google.com/p/fbug/source/browse/branches/firebug1.6/content/firebug/net.js#4388

That's weird; cache entries that got opened for reading should have a device...

Comment 19

[John J. Barton]

Ok here's hint, the crashing entry for the test case from comment 6 is a swf file:
extensions.firebug: net.onCacheEntryAvailable for file.href=http://www.caterstop
.com/App_Themes/catering/img/caterstop-catering-equipment-logo-trans.swf

Comment 20

[Johnny Stenback (:jst)]

Blocking, primarily because this is a firebug-p1 bug.

Comment 21

[timeless]

Attachment: smaller test

Comment 23

[Allan Beaufour]

(In reply to comment #22)
> *** Bug 546851 has been marked as a duplicate of this bug. ***

Note that that bug has a 100% reproducible crash with Firebug and a clean cache here:
http://beaufour.dk/blog_crash

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/dbb5dd6ea250

Comment 25

[Zack Weinberg (:zwol)]

Comment on attachment 427681 [details] [diff] [review]
smaller test

This seems to have fallen through the cracks.  Requesting approval for 1.9.2.3.

Comment 26

[chris hofmann]

#14 topcrash in early firefox 3.6.2 crash data so getting this in to 1.9.2.3 would be good.

Comment 27

[timeless]

chofmann: i doesn't have approval... anyway, once it gets approval, someone should push it. i'm unlikely to do so for roughly a month.

Comment 28

[John J. Barton]

> This seems to have fallen through the cracks.  Requesting approval for 1.9.2.3.

This request seems not to have been set, so I am setting it. By FF 3.6.3 the patch from comment 24 would be baked well on trunk.

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #28)
> > This seems to have fallen through the cracks.  Requesting approval for 1.9.2.3.
> 
> This request seems not to have been set

It's on the patch.

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 427681 [details] [diff] [review]
smaller test

a=beltzner for 1.9.2.3

Comment 31

[Zack Weinberg (:zwol)]

I would land this, but I'm on vacation and don't have a good enough connection to do so.

Comment 32

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a543e44567de

Comment 33

[John J. Barton]

I installed Firefox 3.6.3 and crashed it with a stack the points to this bug:
http://crash-stats.mozilla.com/report/index/085eeca2-d24d-4fb8-ad3a-3fcc72100405
see
http://code.google.com/p/fbug/issues/detail?id=2984

Comment 34

[timeless]

john: please read the bugzilla flags for bugs before complaining in them.

this one has status1.9.2: .4-fixed

(the approval flag on the attachment is approval1.9.2.4+ too)

Comment 35

[Al Billings [:abillings - ex-MoCo]]

Marking as verified for 1.9.2 based on the passing checked in test in lieu of manual reproduction steps for the bug. 

John, this won't be fixed in a shipped release until Firefox 3.6.4 is out.