786687 - crash in NS_strlen with SEO Toolbar 2.4

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Scoobidiver (away)]

It's currently #24 top browser crasher in 15.0.
It first appeared in 15.0a1/20120604 and is likely a regression from bug 743581.

It's correlated to SEO Toolbar:
  NS_strlen(wchar_t const*)|EXCEPTION_ACCESS_VIOLATION_READ (42 crashes)
    100% (42/42) vs.   0% (42/72157) [e-mail] (2.4)

According to comment, opening a new tab is enough to make it crash.

Signature 	NS_strlen(wchar_t const*) More Reports Search
UUID	dad48ee5-b588-47c3-aa83-d7ca52120829
Date Processed	2012-08-29 14:57:27
Uptime	146
Last Crash	2.7 minutes before submission
Install Age	1.5 hours since version was first installed.
Install Time	2012-08-29 13:27:51
Product	Firefox
Version	15.0
Build ID	20120824154833
Release Channel	release
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 42 stepping 7
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x1002, AdapterDeviceID: 0x6779, AdapterSubsysID: 00000000, AdapterDriverVersion: 8.982.0.0
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
EMCheckCompatibility	True
Adapter Vendor ID	0x1002
Adapter Device ID	0x6779
Total Virtual Memory	4294836224
Available Virtual Memory	3818721280
System Memory Use Percentage	32
Available Page File	17756868608
Available Physical Memory	5750181888

Frame 	Module 	Signature 	Source
0 	xul.dll 	NS_strlen 	obj-firefox/xpcom/build/nsCRTGlue.cpp:73
1 	xul.dll 	nsTextToSubURI::ConvertAndEscape 	intl/uconv/src/nsTextToSubURI.cpp:48
2 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
3 	xul.dll 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2442
4 	xul.dll 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1500
5 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:313
6 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2515
7 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:329
8 	mozjs.dll 	js::Invoke 	js/src/jsinterp.h:125
9 	mozjs.dll 	js_fun_apply 	js/src/jsfun.cpp:735
10 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:313
11 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2515
12 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:329
13 	mozjs.dll 	array_readonlyCommon<ArrayForEachBehavior> 	js/src/jsarray.cpp:3231
14 	mozjs.dll 	js::GetPropertyOperation 	js/src/jsinterpinlines.h:227
15 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2515
16 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:329
17 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:361
18 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5549
19 	xul.dll 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:1899
20 	xul.dll 	nsJSEventListener::HandleEvent 	dom/src/events/nsJSEventListener.cpp:191
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=NS_strlen%28wchar_t+const*%29

Comment 1

[Andrew Quartey [:drexler]]

Attachment: patch

I was able to reproduce this consistently. Underlying cause for crash is the original null check for nsCRT::strlen was removed by the bug 743581. The current usage of NS_strlen assumes that the given string is non-empty. If this is intentional, then a local null-check where it's called is required, otherwise this patch nips it in the bud.

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 656564 [details] [diff] [review]
patch

Review of attachment 656564 [details] [diff] [review]:
-----------------------------------------------------------------

null is not a valid argument to strlen.  Other NS_ string functions also will crash if you pass null.

I'd much rather us fix up the call sites.

Comment 3

[Scoobidiver (away)]

Copying Jorge to warn the add-on developer.

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

This add-on doesn't have any binaries and I wouldn't expect it to have any unusual code. Do we know what kind of code is causing this crash?

Comment 5

[Andrew Quartey [:drexler]]

Attachment: patch

Manually went through all the call sites for NS_strlen. This was the only one without a form of null-check prior to use.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 657349 [details] [diff] [review]
patch

Review of attachment 657349 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/uconv/src/nsTextToSubURI.cpp
@@ +44,5 @@
>         if(NS_SUCCEEDED(rv))
>         {
>            char buf[256];
>            char *pBuf = buf;
> +          int32_t ulen = text? NS_strlen(text) : 0;

put a space after text.

int32_t ulen = text ? NS_strlen(text) : 0;

Comment 7

[Andrew Quartey [:drexler]]

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=22133598589f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22133598589f