147720 - M1RC3 crash [@ nsCharTraits<unsigned short>::length]

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[ly nguyen]

i've tried to reproduce the crash using rc3 and user comments as a guide but was
unsuccessful. below is the latest info from talkback:

     Count   Offset    Real Signature
[ 4   nsCharTraits<unsigned short>::length 00a06a8e - nsCharTraits<unsigned
short>::length ]
[ 1   nsCharTraits<unsigned short>::length e1d36f92 - nsCharTraits<unsigned
short>::length ]
 
     Crash date range: 2002-05-24 to 2002-05-27
     Min/Max Seconds since last crash: 2010 - 84541
     Min/Max Runtime: 7428 - 84541
     Keyword List :  
     Count   Platform List 
     4   Windows NT 5.1 build 2600
     1   Windows NT 5.0 build 2195
 
     Count   Build Id List 
     5   2002052308
 
     No of Unique Users         4
 
 Stack trace(Frame) 

	 nsCharTraits<unsigned short>::length	[..\..\dist/include/string\nsCharTraits.h  line 201] 
	 nsDependentString::Rebind
[..\..\dist/include/string\nsDependentString.h  line 60] 
	 nsAString::do_AppendFromElementPtr
[d:\builds\seamonkey\mozilla\string\src\nsAString.cpp  line 360] 
	 nsAutoString::nsAutoString
[d:\builds\seamonkey\mozilla\string\obsolete\nsString2.cpp  line 1202] 
	 nsMetaCharsetObserver::Notify
[d:\builds\seamonkey\mozilla\intl\chardet\src\nsMetaCharsetObserver.cpp  line 280] 
	 nsMetaCharsetObserver::Notify
[d:\builds\seamonkey\mozilla\intl\chardet\src\nsMetaCharsetObserver.cpp  line 165] 
	 nsObserverEntry::Notify
[d:\builds\seamonkey\mozilla\htmlparser\src\nsDTDUtils.cpp  line 1578] 
	 HTMLContentSink::NotifyTagObservers
[d:\builds\seamonkey\mozilla\content\html\document\src\nsHTMLContentSink.cpp 
line 3953] 
	 CNavDTD::WillHandleStartTag
[d:\builds\seamonkey\mozilla\htmlparser\src\CNavDTD.cpp  line 1400] 
	 CNavDTD::HandleStartToken
[d:\builds\seamonkey\mozilla\htmlparser\src\CNavDTD.cpp  line 1649] 
	 CNavDTD::HandleToken
[d:\builds\seamonkey\mozilla\htmlparser\src\CNavDTD.cpp  line 908] 
	 CNavDTD::BuildModel
[d:\builds\seamonkey\mozilla\htmlparser\src\CNavDTD.cpp  line 521] 
	 nsParser::BuildModel
[d:\builds\seamonkey\mozilla\htmlparser\src\nsParser.cpp  line 1869] 
	 nsParser::ResumeParse
[d:\builds\seamonkey\mozilla\htmlparser\src\nsParser.cpp  line 1733] 
	 nsParser::OnDataAvailable
[d:\builds\seamonkey\mozilla\htmlparser\src\nsParser.cpp  line 2369] 
	 nsDocumentOpenInfo::OnDataAvailable
[d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp  line 245] 
	 nsStreamListenerTee::OnDataAvailable
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsStreamListenerTee.cpp  line 98] 
	 nsHttpChannel::OnDataAvailable
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpChannel.cpp  line
2965] 
	 nsOnDataAvailableEvent::HandleEvent
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsStreamListenerProxy.cpp  line 203] 
	 PL_HandleEvent
[d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c  line 597] 
	 PL_ProcessPendingEvents
[d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c  line 530] 
	 _md_EventReceiverProc
[d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c  line 1078] 
	 USER32.dll + 0x3c076 (0x77d7c076)  
	 USER32.dll + 0x3c076 (0x77d7c076)  
	 _except_handler3()  
	 kernel32.dll + 0x3bb86 (0x77e9bb86)   
 
     (6693358)	URL: http://www.nytimes.com
     (6693358)	Comments: I had several tabs open and I clicked on a new link inside one of
the tabbed windows to load a new page.
     (6685112)	URL: freshmeat.net
     (6685112)	Comments: Opening URL  from freshmeat.net causes crash

Comment 1

[timeless]

.

Comment 2

[ly nguyen]

Attachment: another stack trace for [@ nsCharTraits<unsigned ]

Comment 3

[jag (Peter Annema)]

nsCharTraits<PRunichar>::length(ptr) can crash for a few reasons.

One is that |ptr| is |0|, another is that |ptr| points to memory we're not
allowed to read. In this case I suspect it's the latter that's happening.

279              nsAutoString contentPart1(contentValue+10); // after
"text/html;"

Except the test above is for "text/html" so +10 may put it right after the
null-terminator. I suspect changing it to +9 will fix this. Patch coming up.

Comment 4

[jag (Peter Annema)]

Attachment: Fix

Untested patch, the second chunk is the fix, the first chunk is a
simplification of the test.

Comment 5

[jag (Peter Annema)]

Nominating for nsbeta1.

Comment 6

[timeless]

while i don't think this patch is correct, i don't believe the original code 
was either (ignoring crashes).

offhand, i've seen this crumy code before, and i'm pretty sure i've complained 
about it before.

fwiw http://www.ietf.org/rfc/rfc2045.txt specifies content type rules http://
www.ietf.org/rfc/rfc2234.txt describes abnf
section 3.1 covers 'LINEAR WHITE SPACE:'
so what we should do is RFind, and then verify that the preceding character is 
a space, semicolon or tab.

Comment 7

[timeless]

examples of inputs that the current and proposed code don't handle correctly:
'text/html; charset=LATIN-1; foocharset=US-ASCII'
in fact, we should probably tokenize around ';' or scan forward for it.

Comment 8

[jag (Peter Annema)]

This patch fixes the crash, and throws in a simplification of code for free. No,
it doesn't make Mozilla perfect, but I never said it would do that. Yes, we
should probably be a little smarter in the parsing, but right now the code works
with what the webpages provide, since there is no site out there that'll have a
"foocharset" (because there's no point in putting that in). File another bug on
the parsing if you must, leave this bug to address this crash.

Comment 9

[John Morrison]

Lots of instances of this crash showing up for nsCharTraits::length in each 
of the mozilla release candidates, and continuing on the branch and trunk.

If this patch is good, then let's move forward on landing it to trunk. 

Comment 10

[Blake Ross]

Comment on attachment 85389 [details] [diff] [review]
Fix

sr=blake

Comment 11

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

Comment on attachment 85389 [details] [diff] [review]
Fix

r=dbaron.  If you want to be pedantic, the following is incorrect:

>+         Substring(httpEquivValue,
>+                   httpEquivValue+contenttype.Length()).Equals(contenttype,
>+                                                               nsCaseInsensitiveStringComparator())
>+         &&
>+         Substring(contentValue,
>+                   contentValue+texthtml.Length()).Equals(texthtml,
>+                                                          nsCaseInsensitiveStringComparator())

Since you don't know the lengths of contentValue or httpEquivValue, but you'll
probably get away without crashing.  It's worth testing with short strings,
though.

Comment 12

[jag (Peter Annema)]

Yeah, I saw that when I read the code. It's ugly, but it works as is because
contentValue and httpEquivValue are null terminated, which'll guarantee we won't
try to read beyond the null (since we know that the strings we're comparing
against don't have embedded nulls).

Comment 13

[jag (Peter Annema)]

Attachment: Suggested fix for branch

Comment 14

[jag (Peter Annema)]

Comment on attachment 88592 [details] [diff] [review]
Suggested fix for branch

Carrying over r and sr since this is exactly the second hunk of the trunk patch

Comment 15

[jag (Peter Annema)]

Taking bug. Adding nsbeta1 and adt1.0.1, I think we really want the small fix on 
the branch, if it indeed fixes this crash (I'm pretty sure it does).

Since we don't have a testcase, let's see if the fix on the trunk removes this 
stack from trunk talkback data. John, can you keep an eye on that?

Comment 16

[jag (Peter Annema)]

And marking fixed since I checked it in on the trunk today.

Comment 17

[John Morrison]

will do.

Comment 18

[Yuying Long]

Change QA contact to [e-mail] per comment #15, cause there is not way
for me to verify it.

Comment 19

[scottputterman]

adding adt1.0.1+.  Please get drivers approval and land before the 6/25 branch
build.

Comment 20

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 21

[John Morrison]

Verified fixed for the trunk. No crashes reported for trunk builds
with this stack trace since 06/18, after 29 crashes reported 06/09 to
06/18 (3 per day).

Comment 22

[Olivier Cahagne]

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

Comment 23

[lchiang]

What is the signature to look at for the branch builds in talkback?
nsCharTraits<unsigned short>::length ?

Comment 24

[John Morrison]

talkback gets a little baffled by C++ template syntax. I use just 
'nsCharTraits' to get a match. There are sporadic cases of a crash 
ending in nsCharTraits::<unsigned short>length on the trunk, although
not apparently the same crash as this one (the bug is really with the
callers, not 'length'). There weren't any crashes on the branch in 'length'
until a couple of days ago when junruh had 8 crashes while doing something
with certificates (according to the comments). I don't know if a bug was 
opened for those crashes, but it would be a different bug than this one.
As for this bug, per talkback and the checkin log (1.64.2.3 of 
nsMetaCharsetObserver.cpp), verified for the branch.

Comment 25

[John Morrison]

Doh, meant to cc: junruh on the previous comment just to ask if there was a bug 
on a file for a crash in |nsCharTraits::<unsigned short>length| when working 
with certificates (since you had those talkback crashes in the past few days).

Comment 26

[John Unruh]

I believe that bug 152382 causes the crash in the cert manager.

Comment 27

[Jan Carpenter]

Most recent build:  2003011305, reopening. This is currently #19 on the trunk
topcrash list.

nsCharTraits<unsigned   9 
Crash data range: 2003-01-12 to 2003-01-13
Build ID range: 2003011204 to 2003011305
Stack Trace: 

	 nsCharTraits<unsigned short>::length	[../../dist/include/string\nsCharTraits.h
 line 200]
	 nsDependentString::Rebind	[../../dist/include/string\nsDependentString.h  line 60]
	 nsAString::do_AssignFromElementPtr
[c:/builds/seamonkey/mozilla/string/src/nsAString.cpp  line 303]
	 nsMIMEInfoImpl::SetDefaultDescription
[c:/builds/seamonkey/mozilla/netwerk/mime/src/nsMIMEInfoImpl.cpp  line 295]
	 XPTC_InvokeByIndex
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp 
line 102]
	 XPCWrappedNative::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp  line 2025]
	 XPC_WN_GetterSetter
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp 
line 1317]
	 js_Invoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 841]
	 js_InternalInvoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 932]
	 js_Interpret	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 2648]
	 js_Invoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 857]
	 js_InternalInvoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 932]
	 JS_CallFunctionValue	[c:/builds/seamonkey/mozilla/js/src/jsapi.c  line 3433]
	 nsJSContext::CallEventHandler
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp  line 1043]
	 GlobalWindowImpl::RunTimeout
[c:/builds/seamonkey/mozilla/dom/src/base/nsGlobalWindow.cpp  line 4749]
	 GlobalWindowImpl::TimerCallback
[c:/builds/seamonkey/mozilla/dom/src/base/nsGlobalWindow.cpp  line 5105]
	 nsTimerImpl::Fire	[c:/builds/seamonkey/mozilla/xpcom/threads/nsTimerImpl.cpp 
line 383]
	 nsAppShell::Run	[c:/builds/seamonkey/mozilla/widget/src/windows/nsAppShell.cpp
 line 176]
	 nsAppShellService::Run
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp  line 471]
	 main1	[c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp  line 1559]
	 main	[c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp  line 1907]
	 WinMain	[c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp  line 1927]
	 WinMainCRTStartup()
	 kernel32.dll + 0x214c7 (0x77e814c7)
 
 	Source File : ../../dist/include/string/nsCharTraits.h line : 200

Comment 28

[Jay Patel [:jay]]

Closing this bug again as fixed.   Jan, the stack looks a bit different.  I
think we should log a new bug if we continue to see these crashes.

Comment 29

[Jay Patel [:jay]]

v.