98182 - M094 & Trunk crashes exiting browser [@ PK11_GetInternalSlot][turbo]

Status: VERIFIED DUPLICATE of bug 101329

(Core Graveyard :: Security: UI, defect, P1)

Description

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

Starting in the 2001-08-30-05 build there were a bunch of crashes on Windows
reported in Talkback with the following stack (reason: Access Violation):

PK11_GetInternalSlot()
PK11_TokenExists()
ssl3_config_match_init()
ssl2_HandleServerHelloMessage()
ssl2_BeginClientHandshake()
ssl_Do1stHandshake()
ssl_SecureSend()
ssl_SecureWrite()
SSL_ImportFD()
nsSSLIOLayerWrite
[d:\builds\seamonkey\mozilla\security\manager\ssl\src\nsNSSIOLayer.cpp, line 704]

(I'm guessing there aren't line numbers because the NSS build system on Windows
is doing the Windows equivalent of stripping libraries itself.)

I'm not sure what changed around that time, or whether it's in PSM or NSS, or what.

Comment 1

[Wan-Teh Chang]

Nelson, can you see anything obviously wrong with this
stack trace?  If not, I will change the product to Browser
for further investigation.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Yes, I see something wrong with this trace. 
SSL_ImportFD() does not do any I/O.  
It does not call SSL_SecureWrite().

I'm guessing that this is a developer's personal build, and that
he's forgotten that gmake dependencies don't work properly on windows,
or that CVS update doesn't change the header files in dist (unlike Unix,
where they're simlinked).  But that's a wild guess.

I'd suggest the developer do a complete clean build of all of NSS,
including gmaking the clean, export, and libs targets (assuming coreconf,
I don't know how to do the equivalent using mozilla's build system).

Comment 3

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

It's very unlikely that this is a developer's personal build -- there were a
number of these crashes reported by different users, entirely outside Netscape
(and therefore with no access to talkback source).  (It might be more likely
that the symbols for a static function are being omitted and you're seeing the
name of the closest previous non-static function.)

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

If we ignore the SSL_ImportFD item in the stack, the rest of the stack is
consistent with a call to SSL_SecureWrite (via PR_Write).  

PK11_GetInternalSlot() is a one-line function that calls 
SECMOD_GetInternalModule() and then immediately references the pointer
returned by SECMOD_GetInternalModule() without checking it first.
If SECMOD_GetInternalModule() returned a null or corrupt pointer, 
this crash would occur.

So, there are two issues here:
1. Why is SECMOD_GetInternalModule() returning a bad value? 
   This should be investigated.  It's a problem with the pk11 wrapper layer.

2. PK11_GetInternalSlot() should check the value returned by 
SECMOD_GetInternalModule() for NULL before using the value, and should 
return NULL if it gets a NULL from SECMOD_GetInternalModule().

I'll make that change (#2) in the trunk.  
I suggest this bug be reassigned to relyea to investigate issue #1.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Avoid crash by detecting null internal slot pointer.

Comment 6

[Robert Relyea]

Get internal module returns a static variable which is either the internal
module or NULL.

If this function is failing it's because NSS was called, but hasn't been
initialized yet, or has been shutdown.

This fix will most likely just move the crash rather than repair it, as most
callers of Get_InternalSlot do not check for failure either.

There are literally dozens of places that may go belly-up if NSS is not
initialized, so unless we find out the root problem with initialization, the
patch won't really prevent any crashes.

bob

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

If indeed the problem is that NSS functions are being called before NSS
has been initialized, then this is an application error, not an NSS 
library error.  But we cannot determine that from the information in
this bug.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I checked in the test for the NULL internal module pointer.
I agree with Bob that the code will probably just crash somewhere else.
But testing it here is still the right thing to do, IMO.

Given that the application is attempting to do SSL, it seems unlikely
to me that the application has made no attempt to initialize NSS.
So, the likely explanations are that the attempt failed, but the 
application ignored the error and went on, or that memory has been 
corrupted after initialization is complete.  Either of these conditions
is probably an error outside of NSS.  

This bug should be reassigned to the PSM group.

Comment 9

[Wan-Teh Chang]

Product => PSM.

Comment 10

[Stephane Saux]

to kai
->2.1
P1
nsenterprise+
Kai, do you think this may have anything to do with the disk full fix?

Comment 11

[(Don't use, old account) - Kai Engert]

The "disk full fix" from bug 76915 was checked in on August 22, but according to
the report the first problems were seen on August 30.

Since August 22 we init NSS in the very early stages of application startup, and
exit the app if that doesn't work.

When I use Bonsai to query what happened around that date to PSM and NSS, I only
find one change that I think could possibly be related. It changes an #ifdef
block to some init code.

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/security/nss/lib/softoken&command=DIFF_FRAMESET&file=pk11db.c&rev1=1.1&rev2=1.2&root=/cvsroot

All other checkins around that date seem to be unrelated. (The full list is at
(slow):
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=%2Fmozilla%2Fsecurity%2Fnss%2F+%2Fmozilla%2Fsecurity%2Fmanager%2F&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=08%2F27%2F2001&maxdate=08%2F30%2F2001&cvsroot=%2Fcvsroot
)

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Kai,  
the link you provided showed diffs between revs 1.1 and 1.2 of pk11db.c.
But that file is a new file on the trunk, and is not part of the client
builds.  It doesn't yet even have the NSS_CLIENT_TAG.  So, that's not it.

Is this problem reproducible?

Comment 13

[Robert Relyea]

pk11db.c was in pk11wrap in NSS 3.3 and before.

It has been moved to softoken in NSS 3.4.

Kai, you aren't pulling from the NSS tip for PSM are you? The NSS tip is
definately not ready for general use (we still have QA failures). In fact if PSM
is pulling from the NSS tip, it is quite likely that you will run into some of
these problems.

I suspect the *real* change that was made was the change from NSS 3.2 to NSS 3.3.

Comment 14

[(Don't use, old account) - Kai Engert]

False alert I think. I checked, both the trunk and the 094 branch have the file
pk11db.c inside pk11wrap. This means they are pulling from NSS 3.3.

I found the above by looking for checkins to the NSS and PSM directories, but
didn't look at the tags.

Comment 15

[Stephane Saux]

I counted 29 TALKBACK reports.  All of them are on the trunk. build dates are
from  8/29 to 09/06.

Comment 16

[Hong Kwon]

per comments, changing to ASSIGNED

Comment 17

[(Don't use, old account) - Kai Engert]

I'm unable to reproduce this.
In the talkbalk reports, I can't find any helpful comments, besides from the
fact that people are in the process of leaving the application.

Therefore I have to try to analyze this by looking at the code:

The crashing function looks like that:

  PK11SlotInfo *
  PK11_GetInternalSlot(void) 
  {
    return PK11_ReferenceSlot(SECMOD_GetInternalModule()->slots[0]);
  }

As the crash is directly within this function, we know for sure that
SECMOD_GetInternalModule() returns. The return value must be either a null (or
invalid) value, or the attribute slots is null (or invalid). Function
PK11_ReferencesSlot can not be have been called, because that would show up in
the stack trace.

SECMOD_GetInternalModule() does simply return the value of variable
internalModule, which is a pointer, limited to the scope of file pk11util.c.

1) Let's assume that internalModule itself is NULL when we crash.

As we know the crash is during application shutdown, it seems likely that the
code is arriving at SECMOD_Shutdown, which destroys the object pointed to by
internalModule and sets this pointer to NULL.

Besides from shutdown, this variable is set only by SECMOD_init (which looks
unlikely as a cause for this bug), SECMOD_SetInternalModule and
SECMOD_DeleteInternalModule.

I can't find code (besides in init code) that calls SetInternalModule.

DeleteInternalModule is called from the NSS command line tools and from PSM
ToggleFIPSMode() only. Looks unlikely to be the cause, too.

The shutdown code is called when the NSS shared library is destructed. Is it
possible, that the PIPNSS component get's unloaded, while the network code still
needs it?

I conclude: The only realistic scenario I can see is: The user selects to quit
the application, but the https / ssl code is still active, and while the app
shuts down, a SSL handshake is still in progress, which causes the crash as the
internal slot has already been killed.

Maybe we should make the network library try to obtain a reference to the crypto
libraries, and, if successful, delay releasing that reference until the final
statement of the network shut down code?


2) Now let's assume that internalModule is valid, but attribute slots is invalid.

But I can't find any code in NSS files where the filename contains "pk11", that
references an attribute "slots" in a way that modifies the pointer value,
besides in the loading code.

There is a call to PK11_FreeSlot, called from SECMOD_DestroyValue, which is
actually called from the destructor of the PSM PKCS11 module.

So again, this could be caused by shutting down of the crypto shared libraries,
while there is still network code active.


The above is pure theory, and maybe I have missed some code fragments while
making my deductions. So I basically hope that my research will give some ideas
to people with deeper knowledge of the codebase, and whether my suggestion with
the reference counter makes sense or seems useless.


I'm looking for help, either by trying to reproduce the bug or by commenting my
above statements.

Comment 18

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

The disassembly of the code around the crash (with the first line being the one
on which the crash occurred -- talback doesn't show before, for some reason) is:

608d2af5 8b4028           mov     eax,[eax+0x28]     <== CRASH HERE
608d2af8 8b08             mov     ecx,[eax]
608d2afa 51               push    ecx
608d2afb e820e3ffff       call    608d0e20
608d2b00 83c404           add     esp,0x4
608d2b03 c3               ret

the EAX register is 0.  This suggests that SECMOD_GetInternalModule is returning
null.  (I assume that the |slots| member is at offset 0x28.)

Comment 19

[(Don't use, old account) - Kai Engert]

nsNSSComponent is declared as:

NS_IMPL_THREADSAFE_ISUPPORTS6(nsNSSComponent,
                              nsISecurityManagerComponent,
                              nsISignatureVerifier,
                              nsIEntropyCollector,
                              nsINSSComponent,
                              nsIObserver,
                              nsISupportsWeakReference);

The destructor of nsNSSComponent calls NSS_Shutdown.
After this has been shut down, no NSS operations are allowed and are prone to crash.


When a SSL socket is opened, the code arrives at nsSSLIOLayerAddToSocket /
nsSSLIOLayerNewSocket, which will create an object of type nsNSSSocketInfo.
This object is referenced by the socket transport code.

NS_IMPL_THREADSAFE_ISUPPORTS4(nsNSSSocketInfo,
                              nsITransportSecurityInfo,
                              nsISSLSocketControl,
                              nsIInterfaceRequestor,
                              nsISSLStatusProvider)

While an nsNSSSocketInfo is in use by the network transport code, this can
result in NSS functions being called - because the SSL code has added itself as
a layer to the network code.
For my understanding this means, as long as there are nsNSSSocketInfo objects
alive, NSS must not be shut down.


But I can't see code that couples nsNSSSocketInfo to nsNSSComponent in terms of
reference counting.

Have I found the cause, or is there a coupling that I don't see?

Comment 20

[Bradley Baetz (:bbaetz)]

nsNSSIOLayer.cpp creates the nsNSSService object in InitNSSMethods(), but never
releases it. So we leak on shutdown. The refcnt never goes to 0, and so
nss_shutdown won't be called.

What am I missing?

Comment 21

[(Don't use, old account) - Kai Engert]

Bradley: You are right with your last statement.

I looked whether NSS_Shutdown is really called and from where. It is called, but
when the application shuts down and cleans up, see the stacktrace below.

#0  nsNSSComponent::~nsNSSComponent (this=0x81e3590, __in_chrg=3) at
../../../../../mozilla/security/manager/ssl/src/nsNSSComponent.cpp:109
#1  0x416603f7 in nsNSSComponent::Release (this=0x81e3590) at
../../../../../mozilla/security/manager/ssl/src/nsNSSComponent.cpp:550
#2  0x401ccc0d in DeleteEntry (aKey=0x8317598, aData=0x8317580, closure=0x0) at
../../../mozilla/xpcom/components/nsServiceManager.cpp:260
#3  0x4017b22a in _hashEnumerateRemove (he=0x83175b8, i=23, arg=0xbffff570) at
../../../mozilla/xpcom/ds/nsHashtable.cpp:381
#4  0x402af2fa in PL_HashTableEnumerateEntries (ht=0x806d90c, f=0x4017b1ec
<_hashEnumerateRemove(PLHashEntry *, int, void *)>, arg=0xbffff570) at
../../../../mozilla/nsprpub/lib/ds/plhash.c:429
#5  0x4017b2eb in nsHashtable::Reset (this=0x806d908, destroyFunc=0x401ccbcc
<DeleteEntry(nsHashKey *, void *, void *)>, aClosure=0x0) at
../../../mozilla/xpcom/ds/nsHashtable.cpp:400
#6  0x4017c9fb in nsObjectHashtable::Reset (this=0x806d908) at
../../../mozilla/xpcom/ds/nsHashtable.cpp:850
#7  0x4017c89a in nsObjectHashtable::~nsObjectHashtable (this=0x806d908,
__in_chrg=3) at ../../../mozilla/xpcom/ds/nsHashtable.cpp:816
#8  0x401ccd9f in nsServiceManagerImpl::~nsServiceManagerImpl (this=0x806d8f0,
__in_chrg=3) at ../../../mozilla/xpcom/components/nsServiceManager.cpp:285
#9  0x401ccf13 in nsServiceManagerImpl::Release (this=0x806d8f0) at
../../../mozilla/xpcom/components/nsServiceManager.cpp:294
#10 0x401cd94d in nsServiceManager::ShutdownGlobalServiceManager (result=0x0) at
../../../mozilla/xpcom/components/nsServiceManager.cpp:544
#11 0x401720cd in NS_ShutdownXPCOM (servMgr=0x0) at
../../../mozilla/xpcom/build/nsXPComInit.cpp:489
#12 0x0805b73d in main (argc=1, argv=0xbffff844) at
../../../mozilla/xpfe/bootstrap/nsAppRunner.cpp:1598
#13 0x405e8e5e in __libc_start_main (main=0x805b4cc <main>, argc=1,
ubp_av=0xbffff844, init=0x8054570 <_init>, fini=0x8067260 <_fini>,
rtld_fini=0x4000d3c4 <_dl_fini>, stack_end=0xbffff83c) at
../sysdeps/generic/libc-start.c:129


But if this is really the only location where NSS_Shutdown is triggered, as the
application code never frees the reference... Is it possible that the socket
transport thread is still running when the application shuts down? Maybe the
security component is freed before the network component(s)?

Comment 22

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

nsNSSModule.cpp is seriously messed up:  it has multiple entries for a given CID
and they're all tied to the nsNSSComponentConstructor but for different
contractIDs.  The GetService into the global in nsNSSIOLayer.cpp is by contract
ID rather than CID -- so it may not work.  If that is the problem (I'm not sure
how the component manager would deal with the strangeness in nsNSSModule.cpp),
then there should be instance counting to release that global when it can be
released, rather than just leaking it.

Comment 23

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

OK, maybe that nsNSSModule.cpp stuff is OK after all.

Comment 24

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

Might the service manager get confused by having multiple ContractIDs and a
single CID for the nsNSSComponent, and create more than one instance?

Comment 25

[Suresh Duddi (gone)]

Where do I find the source for nsNSSComponent.cpp or nsNSSModule.cpp

There are a few components that have more than one contractid for a given cid. I
dont think that is a problem. That was supposed to work by design.

Comment 26

[(Don't use, old account) - Kai Engert]

dp: mozilla/security/manager/ssl/src/

Comment 27

[Stephane Saux]

Attachment: talkback reports as of today with this stack trace.

Comment 28

[Stephane Saux]

Note that links are internal to netscape. Sorry.
This query fails more often that not on climate.mcom.com, so including the saved
output allows one to easily (as long as one has access to the internal network)
access the individual incidents.

There doesn't appear to be any branch builds.

Comment 29

[Jaime Rodriguez, Jr.]

If shiva and jay say this is fixed, then we should put this one on the back burner.

Comment 30

[Jay Patel [:jay]]

This is showing up in the M094 Talkback topcrash report.  Stephane's attached
query does show many of these crashes with build 2001091311 on the Netscape6.20
branch (which also happens to be the Mozilla 0.9.4 release).  Here is the latest
info from Talkback.  The stack doesn't look too helpful, but perhaps the
comments will help us reproduce:

PK11_GetInternalSlot   14
			 98182 	 ASSI 	  	 [e-mail] 2.1 
BBID range: 35353387 - 35494918
Min/Max Seconds since last crash: 256 - 46396
Min/Max Runtime: 256 - 46396
Crash data range: 2001-09-13 to 2001-09-17
Build ID range: 2001091311 to 2001091311

Stack Trace: 

	 PK11_GetInternalSlot()
	 PK11_TokenExists()
	 ssl3_config_match_init()
	 ssl3_HandleRecord()
	 ssl3_HandleRecord()
	 ssl3_HandleRecord()
	 ssl3_HandleRecord()
	 ssl3_GatherCompleteHandshake()
	 ssl_GatherRecord1stHandshake()
	 ssl_Do1stHandshake()
	 ssl_SecureSend()
	 ssl_SecureWrite()
	 SSL_ImportFD()
	 nsSSLIOLayerWrite
[d:\builds\seamonkey\mozilla\security\manager\ssl\src\nsNSSIOLayer.cpp  line 718]
	 SHA1_Update()
	 RNG_UpdateAndEnd_FIPS186_1()
	 prng_RandomUpdate()
	 SocketRecv
[../../../../pr/src/io/prsocket.c  line 641]
	 pl_DefRecv
[../../../../pr/src/io/prlayer.c  line 278]
	 ssl_DefRecv()
	 ssl3_GatherCompleteHandshake()
	 ssl3_GatherCompleteHandshake()
	 0xffffffff
	 0x90909090     (35494918)	Comments: Tried to close Mozilla completely using Mozilla icon in
the task bar. I tried to right click  left click double click  but no menu
appeared. Mozilla opened and I closed it immediately. At this point Mozilla crashed.
     (35484527)	Comments: I just quit  got mesage about icon staying on taskbar  hit OK.
     (35483403)	URL: webmail.cmg.nl
     (35483403)	Comments: I think the web-mail application tried to close the browser
window... It seemed to go slightly wrong.. The app is written in javascript.
     (35482014)	Comments: closed mozilla window while it was receiving data
     (35451055)	URL: http://www.msn.com
(35451055)
Comments: Exiting Hotmail. The page that follows the checkout page was trying to
load after redirect.
     (35445384)	Comments: closed browser (via javascript  apparently)
     (35420953)	URL: www.landbobanken.dk
(35420953)
Comments: Java internet banking

Comment 31

[Stephane Saux]

Kai. So this is still happening on the 0.9.4 branch. From the comments included,
I see two avenues to investigate:
1) using the turbo mode exit.
2) quiting while page is still loading.

Comment 32

[(Don't use, old account) - Kai Engert]

I'm confused why the title of this bug was changed to include M094. My
impression was, from Stephane's attachment, that it crashes on Mozilla trunk,
too. It isn't limited to a branch.

The avenue "still loading while exiting" is indeed the only scenario that makes
sense to me.

I played around on Windows, but I did not see the crash. I did use the turbo
mode feature, which worked stable for me.

Until I can reproduce it myself, the only thing I can try is to add code that
tries to improve thread safetyness of PSM, as well as some other things to test:

I want to test whether the singleton concept of the NSS component is actually
working. The existing code is designed to work as a singleton only. If there
were multiple instances of nsNSSComponent created during runtime, we would
indeed see a crash, as during destruction of the first one, NSS would already
have been shut down, and still open SSL connection would crash in a way as the
above stack trace. However, during my tests so far, I have never seen the
component to be created twice, so this might happen only in an edge condition
that I have not yet come across.

Another, very general, suggestion being made was: the socket information
structure should refcount the nss component in some way, because it depends on
it.

The reason why it currently works: the nsNSSComponent is knowingly leaked. But
this makes it even more incomprehensible why we crash, as the destruction and
NSS shutdown code should not be reached if the singleton concept worked and
there were no multithread issues.

This is flawed, I guess we really should shut down NSS on exit. Currently, we
don't do it, as soon as SSL code has been called at least once during the
Mozilla session.

Comment 33

[Jay Patel [:jay]]

Kai: Sorry for the confusion, I forgot to add "Trunk" to the summary as well. 
We (the Talkback team) usually update the summary to show which
releases/milestones are showing the crash so we can get a quick idea of when we
last saw the crash.  Anyway, I've updated the summary with Trunk, so we know
that this crash is occurring with both Mozilla 0.9.4 and the latest MozillaTrunk
builds.

Comment 34

[Stephane Saux]

Kai: you should try to instantiate multiple instances of NSS explicitly to test
your theory.  If that help reproduce the crash and it's in the same place, that
would probably be significant.

Comment 35

[(Don't use, old account) - Kai Engert]

I made the following change to enforce the crash.

I changed the code receiving the entropy collector in nsGlobalWindow, to not use
do_GetService, but use do_CreateInstance. This will create additional instances
of nsNSSComponent.

Inside the nsNSSComponent, there is an assertion check, that tries to detect if
the init code is called twice. When I use a debug build, the app quits. However,
in an optimized build, I suppose the app will not quit I think.
I therefore disabled the assertion check for testing, too.

When I open the application, create a second window, and surf over https, I
crash very soon. I see the following stack trace, which is at least in the upper
levels similar to the reported stack traces.


#0  0x416f8cc8 in PK11_GetInternalSlot () at pk11slot.c:2132
#1  0x416f8faa in PK11_TokenExists (type=33) at pk11slot.c:2244
#2  0x416b9f32 in ssl3_config_match_init (ss=0x895da00) at ssl3con.c:419
#3  0x416a80ec in ssl2_ConstructCipherSpecs (ss=0x895da00) at sslcon.c:204
#4  0x416ade3a in ssl2_BeginClientHandshake (ss=0x895da00) at sslcon.c:2964
#5  0x416b17bf in ssl_Do1stHandshake (ss=0x895da00) at sslsecur.c:156
#6  0x416b3764 in ssl_SecureSend (ss=0x895da00, buf=0x0, len=0, flags=0) at
sslsecur.c:1075
#7  0x416b3899 in ssl_SecureWrite (ss=0x895da00, buf=0x0, len=0) at sslsecur.c:1109
#8  0x416b6c1c in ssl_Write (fd=0x8938f40, buf=0x0, len=0) at sslsock.c:1232
#9  0x4166e5b7 in nsSSLIOLayerWrite (fd=0x86de738, buf=0x0, amount=0) at
../../../../../mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp:697
#10 0x402c4990 in PR_Write (fd=0x86de738, buf=0x0, amount=0) at
../../../../../mozilla/nsprpub/pr/src/io/priometh.c:141
#11 0x4166d050 in nsNSSSocketInfo::TLSStepUp (this=0x895d968) at
../../../../../mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp:341
#12 0x4166cfcc in nsNSSSocketInfo::ProxyStepUp (this=0x895d968) at
../../../../../mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp:326
#13 0x40ba3e53 in nsHttpConnection::ProxyStepUp (this=0x8963578) at
../../../../../mozilla/netwerk/protocol/http/src/nsHttpConnection.cpp:287
#14 0x40ba38e7 in nsHttpConnection::OnHeadersAvailable (this=0x8963578,
trans=0x89825c8, reset=0xbf7ff654) at
../../../../../mozilla/netwerk/protocol/http/src/nsHttpConnection.cpp:192
#15 0x40ba6f80 in nsHttpTransaction::HandleContentStart (this=0x89825c8) at
../../../../../mozilla/netwerk/protocol/http/src/nsHttpTransaction.cpp:387
#16 0x40ba7318 in nsHttpTransaction::HandleContent (this=0x89825c8,
buf=0x89df960 "HTTP/1.0 200 Connection established\n\nProxy-agent:
Muffin\n\n\n\n", 'Ú' <repeats 140 times>..., count=0, countRead=0xbf7ff7d4) at
../../../../../mozilla/netwerk/protocol/http/src/nsHttpTransaction.cpp:469
#17 0x40ba8016 in nsHttpTransaction::Read (this=0x89825c8, buf=0x89df960
"HTTP/1.0 200 Connection established\n\nProxy-agent: Muffin\n\n\n\n", 'Ú'
<repeats 140 times>..., count=0, bytesWritten=0xbf7ff7d4) at
../../../../../mozilla/netwerk/protocol/http/src/nsHttpTransaction.cpp:777
#18 0x401b2d05 in nsReadFromInputStream (outStr=0x8982924, closure=0x89825cc,
toRawSegment=0x89df960 "HTTP/1.0 200 Connection established\n\nProxy-agent:
Muffin\n\n\n\n", 'Ú' <repeats 140 times>..., offset=0, count=4096,
readCount=0xbf7ff7d4) at ../../../mozilla/xpcom/io/nsPipe2.cpp:830
#19 0x401b27ce in nsPipe::nsPipeOutputStream::WriteSegments (this=0x8982924,
reader=0x401b2cd0 <nsReadFromInputStream(nsIOutputStream *, void *, char *,
unsigned int, unsigned int, unsigned int *)>, closure=0x89825cc, count=16384,
writeCount=0xbf7ff89c) at ../../../mozilla/xpcom/io/nsPipe2.cpp:704
#20 0x401b2d59 in nsPipe::nsPipeOutputStream::WriteFrom (this=0x8982924,
fromStream=0x89825cc, count=16384, writeCount=0xbf7ff89c) at
../../../mozilla/xpcom/io/nsPipe2.cpp:838
#21 0x40b74361 in nsStreamListenerProxy::OnDataAvailable (this=0x8963508,
request=0x89825c8, context=0x0, source=0x89825cc, offset=0, count=16384) at
../../../../mozilla/netwerk/base/src/nsStreamListenerProxy.cpp:288
#22 0x40ba656a in nsHttpTransaction::OnDataReadable (this=0x89825c8,
is=0x89391d0) at
../../../../../mozilla/netwerk/protocol/http/src/nsHttpTransaction.cpp:178
#23 0x40ba57e7 in nsHttpConnection::OnDataAvailable (this=0x8963578,
request=0x86d6020, context=0x0, inputStream=0x89391d0, offset=0, count=8192) at
../../../../../mozilla/netwerk/protocol/http/src/nsHttpConnection.cpp:702
#24 0x40b68ee4 in nsSocketReadRequest::OnRead (this=0x86d6020) at
../../../../mozilla/netwerk/base/src/nsSocketTransport.cpp:2723
#25 0x40b63894 in nsSocketTransport::doReadWrite (this=0x8805eb8,
aSelectFlags=1) at ../../../../mozilla/netwerk/base/src/nsSocketTransport.cpp:999
#26 0x40b62072 in nsSocketTransport::Process (this=0x8805eb8, aSelectFlags=1) at
../../../../mozilla/netwerk/base/src/nsSocketTransport.cpp:480
#27 0x40b6a37a in nsSocketTransportService::Run (this=0x8116820) at
../../../../mozilla/netwerk/base/src/nsSocketTransportService.cpp:458
#28 0x401d6832 in nsThread::Main (arg=0x810fd78) at
../../../mozilla/xpcom/threads/nsThread.cpp:105
#29 0x402ea93a in _pt_root (arg=0x810d530) at
../../../../../mozilla/nsprpub/pr/src/pthreads/ptthread.c:214
#30 0x4030c078 in pthread_start_thread_event (arg=0xbf7ffc00) at manager.c:262

Comment 36

[(Don't use, old account) - Kai Engert]

I just saw this for the very first time! I think I know what causes this
problem. NSS is indeed shut down and restarted during the session, and it has to
do with the -turbo mode on Windows.

The reason is: method nsNSSComponent::Observe is called, which I did not expect.
The "profile changed" event (causing NSS to shut down) seems to be triggered
while there are still connections in progress. This method needs to be redesigned.

More to follow.

Comment 37

[Suresh Duddi (gone)]

nice finding kai. Ccing Conrad "Turbo" Carlen.

Comment 38

[Peter Trudelle]

Nominating for turbo short list.

Comment 39

[(Don't use, old account) - Kai Engert]

Please see also some comments in bug 99566. 

We have learned that the crash is caused by unloading and loading the profile.
PSM tries to shut down and re-init NSS when it is informed by profile change
events. 
While this is done, it is possible that there are still active threads accessing
NSS, e.g. the network communication thread.

The code necessary to add protection is not a small patch, but might require
many changes all over PSM, to safeguard for the "NSS currently unavailble"
condition, i.e. checking for this flag, including to add locks for serializing
access.

While it is a general requirement to have the functionality to switch profiles
(including the certificate databases used by Mozilla), this might require too
many changes, which may be allowed to be put to the 094 branch.

There is a workaround for applications that don't support real profile switching
yet. We could patch PSM to completely ignore the profile loading / unloading
event, by assuming there won't be a switch to a different profile (with
different certificate database files). I'd like to suggest this patch for
applications that are created from the 094 branch. If this restriction isn't
possible, random crashes are always possible. We could try to minimize the
likelyhood for a crash, by delaying NSS_Shutdown until the moment where we need
to re-initialize. I have no idea, whether there might still be SSL sockets
active when the new profile is being loaded/activated. This could result in a
newly activated NSS session accessing NSS library instances from a previous session.

Here are some comments for a correct implementation for the trunk:

nsNSSComponent methods seem to be called from multiple threads, the UI thread,
the observer notifier (not sure which this is), the socket thread. I think we
should protect the flag, whether NSS is currently activated or not, using a mutex.

We need to behave gracefully. But doing so in nsNSSComponent is not enough. The
SSL code adds a function layer to the socket file descriptors. And those
obviously are active in the middle of profile change. And those call global
functions (e.g. nsNSSIOLayerWrite). In all the layer functions, we must check
for NSS being currently in good state (by querying the
nsNSSComponent::mNSSInitiliazed flag).

However, what happens, if a SSL socket layer function is currently active, while
NSS is being shut down? Obviously we have the likelyhood that NSS goes away in
the middle of a socket operation. This means that it doesn't matter whether NSS
(itself) is thread safe or not. Access to NSS functions is asynchronous. If we
want to fail gracefully if NSS is not available, that means, make the check for
"NSS is available" right before every single call to any NSS function, and
protect the calls using the same Mutex as we must use for NSS shutdown and init,
and to protect the member variables of the nsNSSComponent.

We might want to discuss whether we really need to protect every single function
call to NSS or whether we can reduce this.

However, I fear the likelyhood for being required to protect every single
function is high, as indicated by my observation, that I saw a "certificate
domain mismatch" prompter dialog coming up, nearly at the same moment as the
profile observer was called with the "profile unloaded" event.

Comment 40

[(Don't use, old account) - Kai Engert]

Oops, in the previous comment, in the fifth paragraph, there is the important
word NOT missing:


While it is a general requirement to have the functionality to switch profiles
(including the certificate databases used by Mozilla), this might require too
many changes, which may be *NOT* allowed to be put to the 094 branch.

Comment 41

[Stephane Saux]

Since this concerns general architecture of the security subsystems, I'm cc'ing
Terry Hayes. 

Comment 42

[Suresh Duddi (gone)]

Cant NSS not shutdown, but just forget all cert db etc and reinitialize with the
new profile's cert db etc...in this situation ?

Comment 43

[(Don't use, old account) - Kai Engert]

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

Comment 44

[Robert Relyea]

I'm a little concerned that the client is not trying to quiesce all its
connections before it tries to switch profiles. The Initialization and shutdown
code in NSS is not thread safe. This is the code where any global locks which
may be needed are acquired.

It seems to me that it's a dangerous thing in general to allow operations to
continue while the profile is in transition. Mail/News components can't be happy
if  you are reading a message in the middle of reading a message.

Making PSM fail when NSS isn't initialized seems like a hack. The real question
why are there outstanding NSS requests out there to begin with. How is the
profile switch supposed to happen? Does the profiler shutdown each component
first before it starts enabling new components? If so, then you need to make
sure PSM is shutdown after all the potential callers of PSM, and that include
the networking library! PSM should then be brought up before any new potential
callers.

Comment 45

[Robert Relyea]

The database and keys are an integral part of NSS, NSS can't run without them
defined. Not only that, but what tokens you have defined is stored in your
profile. You could have a completely different set of hardware tokens defined.
Switching profiles is a big deal. Before this design Netscape switched profiles
by Shutting down. The profile switching code should assume that it's has to do
most of the work of shutdown.

Even if PSM fails on NSS connections in between the shutdown and the new
initialize, I don't know what kind of results we are going to get if we have
things like SSL connections that start on one profile and finish on the second.
All of these connections have to be closed down before you shutdown NSS.

Comment 46

[(Don't use, old account) - Kai Engert]

Bob, thanks for the explanation.

Maybe this is even the simplest strategy to solve our problem.

Can we use code, that really assures that all network activity has been shut
down completely (even if this blocks), before we try to switch the profile?

Comment 47

[Suresh Duddi (gone)]

Adding in mscott.

Darin/mscott: any way we can tear down network connections completely on a
profile switch.

Comment 48

[Conrad Carlen]

As the first phase of profile switching, all windows are destroyed which cancels
any current loads. The canceling, though, is not synchronous. It's hard to know
when all activity is stopped and trying to enforce that in something as large as
mozilla isn't the right way to go. Whatever a component loads from a profile,
it's given notice that that data is going away. It needs to do what it must to
put itself in a safe state. localstore, for instance, makes a temporary
in-memory data source so it can function without any of the user-specific data
in the meanwhile. How can NSS require, or else crash, that it has this data?
What if it's internal data structures were just empty, in the state they were
before it inernalized any data from the profile? Would it just crash? What if it
failed to read the data for some other reason? I think, from another bug, it'll
crash then too.

This caused a similar problem in the disk cache. There, it was agreed that the
disk cache should be able to handle the situation without complete quiesense.

Comment 49

[Conrad Carlen]

That said, I think having multiple profiles in turbo and therefore switching
should be disabled until this can be worked out. Now that it's out of an
embedding-only context, too many problems like this are cropping up.

Comment 50

[Robert Relyea]

The profile *DEFINES* NSS's internal data structures. What modules are 'loaded'.
What slots those modules have, etc are defined by secmod.db. It's not that the
data structures are empty, they flat out don't exist! You cannot do anything in
NSS without a token defined: not hashing, not PRNG, not anything!

If NSS_Initialize fails, the application cannot call any other NSS function
without crashing. This has been true since the very beginning, and all NSS
applications (including communicator and the servers)live with this restriction.
The reason for this is most of these products have not restricted themselves to
a small set up public API's. NSS can't  make initialization checks on *every*
interface that PSM calls currently because it calles to many internal only
interfaces.

NSS requires it because it has never been a design point that it didn't. When
the shutdown feature was request, I clearly stated what the restrictions were.
Obviously those restrictions weren't communicated back up.

There is a more fundamental problem, though. Even if NSS allow 'changing the
profile on the fly' with it's connections, there are security issues. What if a
connection was created before you switched profiles, managed not to make any NSS
calls until after the profile switch, what is the semantic of that connection?
All the security checks were done in reguard to a completely separate
connection?  These really need to be quiesced before you bring up a new security
context.

Comment 51

[Robert Relyea]

Reading Conrad's comments more carefully, I would say that NSS should not
shutdown on the first phase. We have an issue if we need to access the key or
certdb in that time, but NSS will fail, not crash in that case (if the key and
cert DB go away after initialization, NSS does handle that case 'gracefully').
The user key and cert Data is already squirrelled away currently. There may be
an issue if we have modified the key and cert DB and the dbclose doesn't happen.
NSS will still have the files open at this point, I don't know what issue that
will cause. The potential db corruption in this case is already an issue we have
filed against NSS (it's really a bug in DBM where db_sync doesn't always do what
it should). The secmod.db is openned, read, then closed. As long as someone
doesn't try to add a module in a middle, it could disappear of the face of the
earth and we would never know.

In this case, KAI, we should do the shutdown just before we do the initialize.
We still need to make sure no one is going to call NSS between the two calls. I
am still concerned about the security implications of having connections
starting in one profile and living through the profile switch process.

bob

Comment 52

[Darin Fisher]

i haven't been following all of the discussion on this bug, but in answer to
dp's question: it is possible to "go offline" and thereby force all socket
connections to close (they will not close synchronously, however).

Comment 53

[(Don't use, old account) - Kai Engert]

Bob, you say that there is an issue if dbclose is not called.

Well, then we might have another problem. Currently, in most cases, the PSM code
will not call NSS_Shutdown on application exit.

If there has been at least on SSL connection during an application session, the
component responsible for shutting it down, is intentionally leaked.

This is a different bug. The network layer is not keeping a reference to the
nsNSSComponent. This means, during app shutdown, the destructor of the
nsNSSComponent might be called while there is still network activity. This
destructor has the call to NSS_Shutdown. Not knowing better, I assume the
additional leaking reference count was introduced to prevent a crash.

Bob, if you say that calling NSS_Shutdown is wanted/required, to make sure that
dbclose is called during application exit, I will open another bug in order to
fix this.

Comment 54

[(Don't use, old account) - Kai Engert]

As we need a general solution for profile switching applications, I have created
the new bug 101329.

However, we need a tempory solution for this bug now.


Temporary solution for the 094 branch
=====================================

All suggestions outlined in the new bug 101329 seem to be too risky for the 094
branch.

Conrad suggests to disable profile switching for now.

Still, we might encounter profile loading and unloading events, where the actual
profile will not change, but unloaded/loaded in the turbo mode situation.

To make sure we will not crash if those events occur, I suggest the following
patch. It makes PSM simply ignore the profile change events.

This patch is absolutely wrong for all other applications, but is a good
temporary solution, as it avoids all the problems, by simply not
unloading/reloading NSS.

Comment 55

[(Don't use, old account) - Kai Engert]

Attachment: Suggested temporary fix for 094 branch ONLY.

Comment 56

[Conrad Carlen (not reading bugmail)]

The patch would require that we disable turbo mode for multiple profiles. If
that's what we have to do, I'll disable it. r=ccarlen.

Before taking this step, can we see if there are any talkback incidents of this
on the trunk since bug 99387 was checked in?

Comment 57

[Wan-Teh Chang]

Comment on attachment 50519 [details] [diff] [review]
Suggested temporary fix for 094 branch ONLY.

Kai,

The assumption your patch makes, that the application
will not switch the profile but only unload and reload the same profile again, should be
checked and nsNSSComponent::Observe
should return a failure status or even abort (i.e., assert) if the assumption is not true.

Comment 58

[(Don't use, old account) - Kai Engert]

Attachment: Suggested temporary fix for 094 branch ONLY.

Comment 59

[(Don't use, old account) - Kai Engert]

Updated patch as suggested by wtc. The profile directory will be different when
a different profile is selected. We remember the directory during NSS init, and
compare this when we receive a profile change event.

Comment 60

[(Don't use, old account) - Kai Engert]

Conrad, you ask whether there are still talkback results for this bug.
The answer is yes, I see incident 35726796, which shows the same stack trace.

Comment 61

[Wan-Teh Chang]

+    if (lastProfileDirectory != currentProfileDirectory) {
+      NS_ASSERTION(0, "attempt to switch profile, currently unsupported by PSM");
+      // let's crash in optimized builds
+      *(int*)0 = 1;
+    }

It is a "religious" issue whether we should crash as soon as
we realize that we will crash at some point in the future.
Based on my experience, people's first reaction is to blame
the module that crashed.  Therefore, I suggest that you
rewrite the code fragment above as something like this.

    NS_ASSERTION(lastProfileDirectory == currentProfileDirectory,
        "attempt to switch profile, currently unsupported by PSM");
    if (lastProfileDirectory != currentProfileDirectory) {
        return NS_ERROR_ABORT;
    }

This way it becomes the responsibility of the caller to check your
return value and decide what is the appropriate action.

Comment 62

[Conrad Carlen (not reading bugmail)]

The patch will always cause a crash for anybody who uses the 0.9.4 branch and
profile switching. The crash which caused this bug *may* have happened for them.
But, if they had managed to stop all async activity before the profile switch,
they would not be having this problem. Now, they're guaranteed to have a crash.

But, if you don't wan't the profile to be switched for PSM, just don't register
to observe these two notifications. Then, the problem code will never be hit.

http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsNSSComponent.cpp#801

Comment 63

[(Don't use, old account) - Kai Engert]

(I now see that the command line interface of Mozilla supports selecting a
different profile at runtime, even in turbo mode, which I was not aware of.)

Conrad, you said you want to disable this on the branch. If you do, we will not
introduce a crash using my patch, because the Observe method will never be
triggered with a different profile. Did I understand something wrong?

Using my initial patch, we don't recognize a potential crash, if the patch
accidentially lands in a source tree that does support profile switching.

If we completely disable listening for the profile switch event, we can not
detect at runtime whether NSS switching is required or not, as the application
uses profile switching.

Comment 64

[Conrad Carlen (not reading bugmail)]

> I now see that the command line interface of Mozilla supports selecting a
> different profile at runtime.

If you mean the -P <profile> arg, that only happens:
1) without turbo - only on initial launch, not at runtime
2) with turbo - only if multiple profiles are enabled

Comment 65

[(Don't use, old account) - Kai Engert]

Attachment: New patch for branch only - as suggested by wtc

Comment 66

[Jaime Rodriguez, Jr.]

This fix will not be needed if we disable profile switching in Turbo mode. 

Kai - Pls talk to Conrad.

Comment 67

[Stephane Saux]

This issue is going to be resolved on the branch by providing a fix to bug
101364, which makes applying the the current patch to this bug unnecessary.

This bug should be marked as a duplicate of 101329 which outlines the various
options.

Kai, let me know if that's not the right thing to do.


*** This bug has been marked as a duplicate of 101329 ***

Comment 68

[(Don't use, old account) - Kai Engert]

Jaime, Stephane, I agree to your suggestions. If the profile switch is not
triggered, we don't need a fix for PSM now, and care for it in bug 101329.

Comment 69

[John Unruh]

Verified dupe.