617381 - Race condition between nsNSSSocketInfo::SetNotificationCallbacks and nsNSSSocketInfo::EnsureDocShellDependentStuffKnown

Status: RESOLVED WORKSFORME

(Core :: Security: PSM, defect)

Description

[Honza Bambas (:mayhemer)]

We count on pressumtion EnsureDocShellDependentStuffKnown is always called before SetNotificationCallbacks(null).  But in some cases, rare in release optimized builds, pretty often in debug builds running in a debugger [ lucky, isn't it ?:) ] we may get reversed order.

mCallbacks is also accessed w/o synchronization on two threads, see catched stacktraces (tracepoints in VC++):

Thread: 0x114C _threadstartex
        xul.dll!nsNSSSocketInfo::SetNotificationCallbacks() 
	xul.dll!nsSocketTransport::OnSocketDetached() 
	xul.dll!nsSocketTransportService::DetachSocket() 
	xul.dll!nsSocketTransportService::DoPollIteration() 
	xul.dll!nsSocketTransportService::OnProcessNextEvent() 
	xul.dll!nsThread::ProcessNextEvent() 
	xul.dll!NS_ProcessPendingEvents_P() 
	xul.dll!nsSocketTransportService::Run() 
	xul.dll!nsThread::ProcessNextEvent() 
	xul.dll!NS_ProcessNextEvent_P() 
	xul.dll!nsThread::ThreadFunc() 
	nspr4.dll!_PR_NativeRunThread() 
	nspr4.dll!pr_root() 
	msvcr90d.dll!_callthreadstartex() 
	msvcr90d.dll!_threadstartex() 
	kernel32.dll!761d3677() 
	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]
	ntdll.dll!76ef9d42() 
	ntdll.dll!76ef9d15() 

	this = 0x09d73484 {mRefCnt={...} _mOwningThread={...} mCallbacks={...} ...}


Thread: 0x1258 _threadstartex 	xul.dll!nsNSSSocketInfo::EnsureDocShellDependentStuffKnown() 
	xul.dll!nsNSSSocketInfo::GetExternalErrorReporting() 
	xul.dll!nsNSSBadCertHandler() 
	ssl3.dll!ssl3_HandleCertificate() 
	ssl3.dll!ssl3_HandleHandshakeMessage() 
	ssl3.dll!ssl3_HandleHandshake() 
	ssl3.dll!ssl3_HandleRecord() 
	ssl3.dll!ssl3_GatherCompleteHandshake() 
	ssl3.dll!ssl_GatherRecord1stHandshake() 
	ssl3.dll!ssl_Do1stHandshake() 
	ssl3.dll!ssl_SecureSend() 
	ssl3.dll!ssl_SecureWrite() 
	ssl3.dll!ssl_Write() 
	xul.dll!nsSSLThread::Run() 
	xul.dll!nsPSMBackgroundThread::nsThreadRunner() 
	nspr4.dll!_PR_NativeRunThread() 
	nspr4.dll!pr_root() 
	msvcr90d.dll!_callthreadstartex() 
	msvcr90d.dll!_threadstartex() 
	kernel32.dll!761d3677() 
	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]
	ntdll.dll!76ef9d42() 
	ntdll.dll!76ef9d15() 

	this = 0x09d73480 {mRefCnt={...} _mOwningThread={...} mCallbacks={...} ...}


I just don't get it how it is possible we first report an error to the socket and then get call to ssl_Write that bubbles to the badCertCallback.  This might be problem in the PSM background SSL thread syncing or we simply use bad nsNSSSocketInfo to call back to.

This is present at least in Fifefox 3.5.15.  So no new bug.

Comment 1

[Benjamin Smedberg]

See also bug 636810

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I already have patches in my local queue that remove EnsureDocShellDependentStuffKnown completely, as part of my work on removing XPCOM proxies from PSM.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

EnsureDocShellDependentStuffKnown was removed as part of SSL thread removal / XPCOM proxy removal. We query the callbacks for the docshell info on each access now, and the main thread and the socket transport thread are blocked when doing so. So, there should be no more race condition.