Closed Bug 817568 Opened 7 years ago Closed 4 years ago

crash in ReadInternetOption @ StringCchCopyNW

Categories

(Core :: Networking, defect, P1, critical)

18 Branch
x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox18 --- wontfix
firefox19 + wontfix
firefox20 + fixed
firefox21 --- affected

People

(Reporter: marcia, Assigned: emk)

Details

(Keywords: crash, Whiteboard: [startupcrash])

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-e266e560-e4ed-4c58-91da-838e62121201 .
============================================================= 

Seen while looking at Firefox 18 data. Crash happens more frequently in FF 18.

More reports: https://crash-stats.mozilla.com/report/list?signature=StringCchCopyNW

High correlation to one addon, which appears to be http://www.dealply.com/

StringCchCopyNW|EXCEPTION_ACCESS_VIOLATION_READ (44 crashes)
     48% (21/44) vs.   3% (107/3766) {20a82645-c095-46ed-80e3-08825760534b} (Microsoft .NET Framework Assistant, http://www.windowsclient.net/)
     48% (21/44) vs.   3% (123/3766) {EB9394A3-4AD6-4918-9537-31A1FD8E8EDF}


Frame 	Module 	Signature 	Source
0 	wininet.dll 	StringCchCopyNW 	
1 	wininet.dll 	TransformMD5 	
2 	xul.dll 	ReadInternetOption 	toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp:91
3 	xul.dll 	nsWindowsSystemProxySettings::GetPACURI 	toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp:200
4 	xul.dll 	nsPACMan::ProcessPending 	netwerk/base/src/nsPACMan.cpp:526
5 	xul.dll 	nsPACMan::ProcessPendingQ 	netwerk/base/src/nsPACMan.cpp:489
6 	xul.dll 	nsPACMan::PostQuery 	netwerk/base/src/nsPACMan.cpp:334
7 	xul.dll 	PendingPACQuery::Run 	netwerk/base/src/nsPACMan.cpp:246
8 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:620
9 	xul.dll 	nsThread::ThreadFunc 	xpcom/threads/nsThread.cpp:258
10 	nspr4.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:395
11 	nspr4.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:90
12 	msvcr100.dll 	_callthreadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:314
13 	msvcr100.dll 	_threadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:292
14 	kernel32.dll 	BaseThreadStart 	
https://crash-stats.mozilla.com/report/list?signature=StringCchCopyNW
(In reply to Marcia Knous [:marcia] from comment #0) 
> High correlation to one addon, which appears to be http://www.dealply.com/
> StringCchCopyNW|EXCEPTION_ACCESS_VIOLATION_READ (44 crashes)
>      48% (21/44) vs.   3% (107/3766) {20a82645-c095-46ed-80e3-08825760534b}
> (Microsoft .NET Framework Assistant, http://www.windowsclient.net/)
>      48% (21/44) vs.   3% (123/3766) {EB9394A3-4AD6-4918-9537-31A1FD8E8EDF}
63% of crashes happen with the same user (same install time).

66% of crashes are on Windows Server 2003 so likely corporate users.

It looks like bug 782034.
Component: Extension Compatibility → Networking
OS: Windows NT → Windows Server 2003
Product: Firefox → Core
Summary: crash in StringCchCopyNW → crash in ReadInternetOption @ StringCchCopyNW
It's #65 top browser crasher in 18.0.1 and #20 in 19.0b2 with many duplicates.
OS: Windows Server 2003 → Windows XP
Whiteboard: [startupcrash]
mcmanus, this looks like a PAC bug which could hit us badly in release. Can you take an urgent look?
Assignee: nobody → mcmanus
Priority: -- → P1
This is actually in :emk's OS code.. I think he had a prospective patch that didn't get backported until crash stats were looked at. emk?
Assignee: mcmanus → VYV03354
Dup of bug 829518?
If this is in fact a dupe of bug 829518 (where FF18 was affected), there's a good chance we'll wontfix for FF19. How do you plan to verify emk?
Verify what?
I'm fine uplifting bug 829518 to aurora and beta. It's safe enough for branches.
(In reply to Masatoshi Kimura [:emk] from comment #7)
> I'm fine uplifting bug 829518 to aurora and beta. It's safe enough for
> branches.

In that case, can you please request approval for that?


And also, I'm not sure that patch really helps the crash here, as it landed on the 14 but here are two crashes I just found on crash-stats that ahppened with the Nightly build from the 24th: bp-79685dd7-6011-4b48-8f04-516ad2130125 bp-ed155a53-1a3e-4bc5-91cf-3bdab2130125
> nsWindowsSystemProxySettings.cpp:72
It looks like I need to enclose broader range with __try...__except.
Attachment #708374 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c3c3913e3c
Whiteboard: [startupcrash] → [startupcrash][leave open]
(In reply to Masatoshi Kimura [:emk] from comment #7)
> Verify what?

Verify that this is an issue that already impatcted FF18 (which it appears you already have). Given that, we'll maintain the status quo in FF19 and fix this for the first time in FF20. Please nominate for uplift.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 563169
User impact if declined: High volume crash
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: none
Attachment #709879 - Flags: approval-mozilla-aurora?
Attachment #709879 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Marking as fixed given comment 15.
I am very disappointed in the patches in this bug and bug 829518. SEH from arbitrary crashes is not safe in code such as this, because it can leave structures in half-initialized states, silently leak memory, or even cause deadlocks if there are locked critical sections or other synchronization primitives.

See bug 854176 which suggests that this code should be rewritten using nsINetworkListManager.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> I am very disappointed in the patches in this bug and bug 829518. SEH from
> arbitrary crashes is not safe in code such as this, because it can leave
> structures in half-initialized states, silently leak memory, or even cause
> deadlocks if there are locked critical sections or other synchronization
> primitives.

I just followed what plug-in module does of which you are a peer.
https://mxr.mozilla.org/mozilla-central/search?string=TRY_SAFE

> See bug 854176 which suggests that this code should be rewritten using
> nsINetworkListManager.

What is nsINetworkListManager?
https://mxr.mozilla.org/mozilla-central/search?string=nsINetworkListManager
You'll note that the __try/__except handling was *removed* from plugins long ago, and currently those macros are just used for android reentry handling.

I meant of course INetworkListManager the Windows API mentioned in bug 854176.
we dont need this open with 854176
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [startupcrash][leave open] → [startupcrash]
You need to log in before you can comment on or make changes to this bug.