+++ This bug was initially created as a clone of Bug #704984 +++ (In reply to Honza Bambas (:mayhemer) from Bug #704984 comment #54) > - PSMRunnable (SyncRunnableBase) allowed to be dispatched also from the main > thread (calls its operation method directly) > - NS_IsMainThread() special handling in PK11PasswordPrompt no longer > necessary OK. > - SSLServerCertVerificationJob remembers the thread it has to dispatch the > result to (= the thread where the socket is being called on) > - nsNSSSocketInfo::GetPreviousCert can now be called on the main thread too > - when send operation on a blocking socket returns would-block then if we > are waiting for cert verify, block by looping the event queue and then retry > the send again There is a better fix. When the current thread is not the socket transport thread, then do not use async. certificate path validation. That is, instead of dispatching a SSLServerCertVerificationJob and returning SECWouldBlock, just call AuthCertificate directly. This will work for blocking sockets too. Async. cert validation is only needed for the socket transport thread. Yesterday (last night), I also investigated the technique you propose in this patch, having the certificate validation job remembering the thread it is dispatched from, and then returning to it. The problem is that there is another assumption that the async cert validation code makes: it assumes that it can interrupt any PR_Poll (any blocking socket I/O operation) by dispatching an event to that thread. It isn't clear to me that this is (always) going to work for anything except the socket transport thread. > The true fix should be made in NSS (not for today): when the socket is set > blocking, we must never return would-block since it will be handled as a > fatal error by the calling layer. No idea how to achieve this This > presumption might be wrong for the ssl socket, though.. The cert auth hook must never return SECWouldBlock for a blocking socket.

I will submit two patches: Patch #1 will make SSL connections work correctly in these cases, but cert overrides won't work. Patch #2 will make the cert overrides work. Patch #1 will be ready shortly. Patch #2 will take a little more time.

(In reply to Brian Smith (:bsmith) from comment #0) > There is a better fix. When the current thread is not the socket transport > thread, then do not use async. certificate path validation. That is, instead > of dispatching a SSLServerCertVerificationJob and returning SECWouldBlock, > just call AuthCertificate directly. This will work for blocking sockets too. > Async. cert validation is only needed for the socket transport thread. Are you sure about this? When you will do this on the main thread, then how will cert verification process OCSP requests that must go though the main thread able to process events?

(In reply to Honza Bambas (:mayhemer) from comment #2) > (In reply to Brian Smith (:bsmith) from comment #0) > > Async. cert validation is only needed for the socket transport thread. > > Are you sure about this? When you will do this on the main thread, then how > will cert verification process OCSP requests that must go though the main > thread able to process events? http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCallbacks.cpp?rev=cf0b31ff2b6d#401 I am not sure yet. But, our code for fetching OCSP responses has special support for running on the main thread.

(In reply to Brian Smith (:bsmith) from comment #3) > http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/ > nsNSSCallbacks.cpp?rev=cf0b31ff2b6d#401 > > I am not sure yet. But, our code for fetching OCSP responses has special > support for running on the main thread. Nice, but: http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCallbacks.cpp?rev=cf0b31ff2b6d#386

BTW, with my patch from bug 704984: > xul.dll!nsHTTPDownloadEvent::Run() Line 100 C++ xul.dll!nsThread::ProcessNextEvent(bool mayWait=true, bool * result=0x003a7277) Line 625 + 0x19 bytes C++ xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x007d7d88, bool mayWait=true) Line 245 + 0x17 bytes C++ xul.dll!nsThread::Shutdown() Line 483 + 0xb bytes C++ xul.dll!NS_InvokeByIndex_P(nsISupports * that=0x062eb400, unsigned int methodIndex=6, unsigned int paramCount=0, nsXPTCVariant * params=0x00000000) Line 103 C++ xul.dll!nsProxyObjectCallInfo::Run() Line 182 + 0x2d bytes C++ xul.dll!nsThread::ProcessNextEvent(bool mayWait=true, bool * result=0x003a7363) Line 625 + 0x19 bytes C++ xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x007d7d88, bool mayWait=true) Line 245 + 0x17 bytes C++ xul.dll!nsNSSSocketInfo::BlockOnCertVerification(int bytesWritten=-1) Line 1028 + 0xd bytes C++ xul.dll!PSMSend(PRFileDesc * fd=0x008337e0, const void * buf=0x0b2b34bc, int amount=43, int flags=0, unsigned int timeout=10000) Line 2267 + 0xc bytes C++ nspr4.dll!PR_Send(PRFileDesc * fd=0x008337e0, const void * buf=0x0b2b34bc, int amount=43, int flags=0, unsigned int timeout=10000) Line 226 + 0x1e bytes C nsldappr32v60.dll!prldap_write(int s=1, const void * buf=0x0b2b34bc, int len=43, lextiof_socket_private * socketarg=0x0b27bb88) Line 218 + 0x1a bytes C nsldap32v60.dll!ber_flush(sockbuf * sb=0x0b1b8a48, berelement * ber=0x0b2b3390, int freeit=0) Line 439 + 0x26 bytes C nsldap32v60.dll!nsldapi_send_ber_message(ldap * ld=0x0b1b8858, sockbuf * sb=0x0b1b8a48, berelement * ber=0x0b2b3390, int freeit=0, int epipe_handler=1) Line 473 + 0x11 bytes C nsldap32v60.dll!nsldapi_send_server_request(ldap * ld=0x0b1b8858, berelement * ber=0x0b2b3390, int msgid=1, ldapreq * parentreq=0x00000000, ldap_server * srvlist=0x00000000, ldap_conn * lc=0x0b1cf098, char * bindreqdn=0x0b2c0778, int bind=0) Line 342 + 0x17 bytes C nsldap32v60.dll!nsldapi_send_initial_request(ldap * ld=0x0b1b8858, int msgid=1, unsigned long msgtype=96, char * dn=0x0b1b0890, berelement * ber=0x0b2b3390) Line 148 + 0x2b bytes C nsldap32v60.dll!simple_bind_nolock(ldap * ld=0x0b1b8858, const char * dn=0x0b1b0890, const char * passwd=0x0b1c99d0, int unlock_permitted=1) Line 153 + 0x17 bytes C nsldap32v60.dll!ldap_simple_bind(ldap * ld=0x0b1b8858, const char * dn=0x0b1b0890, const char * passwd=0x0b1c99d0) Line 82 + 0x13 bytes C xul.dll!nsLDAPOperation::SimpleBind(const nsACString_internal & passwd={...}) Line 322 + 0x30 bytes C++ xul.dll!nsAbLDAPListenerBase::OnLDAPInit(nsILDAPConnection * aConn=0x0b1b0b70, unsigned int aStatus=0) Line 302 + 0x35 bytes C++ xul.dll!nsLDAPConnection::OnLookupComplete(nsICancelable * aRequest=0x0b1b7814, nsIDNSRecord * aRecord=0x0b0d4450, unsigned int aStatus=0) Line 663 C++

(In reply to Honza Bambas (:mayhemer) from comment #4) > Nice, but: > http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/ > nsNSSCallbacks.cpp?rev=cf0b31ff2b6d#386 Yes, that will queue an event on the main thread. Then, later, if we are on the main thread, we will run the event loop and then process that event (among others), right?

This patch simply moves the cert override handling from nsNSSIOLayer.cpp to SSLServerCertVerification.cpp. I did this because I thought I was going to need to make many changes to it. However, I realized later that I hardly had to change anything with the cert override handling. But this was a change that Honza had previously recommended anyway, so I left it in. So, if you want, I can redo the next patch so that it doesn't depend on this one.

Note that Part 1 is just the SyncRunnableBase changes from Honza's patch in bug 704984 attachment 583184 [details] [diff] [review].

tested this in the following horrible way: Horrible Test #1 ---------------- This tests verifies that SSL server cert verification, including OCSP fetching, works correctly on the main thread. 1. I configured a new LDAP account with www.paypal.com as the server, on port 443, using SSL. 2. I set breakpoints in a couple of places in internal_send_receive_attempt. 3. I configured Thunderbird to use this "LDAP" server for autocompletion in Options -> Addressing -> Directory Server. 4. I started composing a new email. 5. I started typing an email address in the "To:" field. 6. My breakpoints in internal_send_receive_attempt were hit. 7. I stepped through the code to verify that everything was working as I expected. 8. Of course, later the bind fails, since www.paypal.com isn't really an LDAP server. Horrible Test #2 ---------------- 1. I reconfigured the LDAP settings to use addressbook.mozilla.com with correct settings, using SSL. 2. I started composing a new email 3. I started typing an email address in the "To:" field. 4. I verified that address looks were working correctly. Tomorrow, I will try to create some xpcshell tests that verify this in a less hacky, and automated way. It won't test LDAP, but it will test some other protocol on the main thread that uses OCSP.

This should have been part of Part 3. Once the cert override logic is moved to SSLServerCertVerification.cpp, SSLServerCertVerificationResult doesn't need to be exposed outside of that file anymore.

Brian, p2 patch could not be applied to comm-cetral/mozilla. Please rebase the patches to what the current client.py from comm-cetral checks out. I want to test the patches with my _working_ LDAPS OCSP'ed server.

Comment on attachment 583723 [details] [diff] [review] Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread Review of attachment 583723 [details] [diff] [review]: ----------------------------------------------------------------- I can't superreview my own work.

Patches now apply and fixes the LDAP issue for me.

Comment on attachment 583957 [details] [diff] [review] Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread Honza, I originally asked you for SR for this patch just because I wanted you to look over the things I omitted from your patch. I don't think an SR is needed for this.

You are missing the changes to CertErrorRunnable::Run(). When the LDAP server has a bad certificate, you don't set the error on the socket, so you silently add an exception for all bad certificates when the cert verification doesn't happen on the socket thread, quit bad. The reason is missing call to SSLServerCertVerificationResult::Run() that sets the result of verification on the socket (and cancels it when cert is bad). Just be aware that you should prevent call to SSL_RestartHandshakeAfterAuthCertificate synchronously from the AuthCertificateHook as you could renter the locks, but you know this better then me. During the review (not yet published) I proposed to have a static function that verifies and if bad, handles the bad cert since you, more or less, duplicate the code of SSLServerCertVerificationJob::Run() in AuthCertificateHook. Not good since it may introduce bugs like described above. Setting the verification result on the socket could be done in that method so you may leave CertErrorRunnable simpler. Comm-beta displays the bad cert info dialog and fails the load. Comm-central w/o the patches obviously deadlocks.

Comment on attachment 583955 [details] [diff] [review] Part 2: Fix certificate verification for blocking sockets and sockets not managed by the socket transport service Review of attachment 583955 [details] [diff] [review]: ----------------------------------------------------------------- Dropping r flag, this is just a pre-review anyway. ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +455,5 @@ > { > + // Runs on a certificate verification thread if the verifying a certificate > + // for a connection on the socket transport thread; otherwise, runs on the > + // thread doing the network I/O, which may (but shouldn't) be the main > + // thread. This comment needs a wording check. The "but shouldn't" seems redundant to me here. This patch is fixing exactly this issue. Now any thread can call on cert verification. @@ +734,4 @@ > > nsNSSSocketInfo *socketInfo = static_cast<nsNSSSocketInfo*>(arg); > + > + bool onSTSThread; onNetworkThread ? @@ +747,5 @@ > + PR_SetError(PR_UNKNOWN_ERROR, 0); > + return SECFailure; > + } > + > + SECStatus rv; s/rv/status/ ? @@ +761,5 @@ > + // thread doing the network I/O may not interrupt its network I/O on receipt > + // of our SSLServerCertVerificationResult event, and/or it might not even be > + // a non-blocking socket. > + rv = AuthCertificate(socketInfo, serverCert); > + // XXX: cert overrides are not handled! You should add to this comment that we also block the read/write op until the cert verification is done regardless the socket is blocking or non-blocking.

Comment on attachment 583726 [details] [diff] [review] Part 4: Fix cert error overrides for blocking sockets and sockets not managed by the socket transport service Review of attachment 583726 [details] [diff] [review]: ----------------------------------------------------------------- Same here. ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +281,5 @@ > // SSLServerCertVerificationResult with the error code from PR_GetError(). > SECStatus > HandleBadCertificate(PRErrorCode defaultErrorCodeToReport, > + nsNSSSocketInfo * socketInfo, CERTCertificate * cert, > + const void * fdForLogging, bool isAsync) Indention @@ +456,2 @@ > } > if (NS_FAILED(nrv)) { Blank line before this if () { line please. @@ +1251,5 @@ > + if (rv != SECSuccess && PR_GetError() == 0) { > + NS_ERROR("AuthCertificate or HandleBadCertificate did not set error code"); > + PR_SetError(PR_UNKNOWN_ERROR, 0); > + } > + } This code now duplicates in SSLServerCertVerificationJob::Run(). You should create a static method containing this code that SSLServerCertVerificationJob::Run() would be using.

Comment on attachment 583725 [details] [diff] [review] Part 3: Move cert override logic to SSLServerCertVerification.cpp, r?honzab Review of attachment 583725 [details] [diff] [review]: ----------------------------------------------------------------- I hate checking these patches.. :) r=honzab.

Comment on attachment 583751 [details] [diff] [review] Part 5: Move SSLServerCertVerificationResult to SSLServerCertVerification.cpp [v2] Review of attachment 583751 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab

Comment on attachment 583957 [details] [diff] [review] Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread The final version of this patch will need to land on beta channel as well. See bug 708813 comment 25.

Are we close to landing this? I'm especially concerned about part 1 landing on the beta channel in time for release - as otherwise some people will have broken LDAP in Thundebird. I'm not sure what the effects could be for Firefox users (if any).

Ping

This is the same patch as the first version but with a whitespace change. I understand that there needs to be a change to the way CertErrorRunnable::Run works for the non-STS case, but in other patches in this series I have changed how CertErrorRunnable works for other reasons.

Comment on attachment 590132 [details] [diff] [review] Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread [v2] I'm afraid I cannot review this until I also see the remaining changes to be made and have a complete set of patches to test.

(In reply to Honza Bambas (:mayhemer) from comment #30) > Comment on attachment 590132 [details] [diff] [review] > Part 1: Make SyncRunnableBase work without deadlocking when run on the main > thread [v2] > > I'm afraid I cannot review this until I also see the remaining changes to be > made and have a complete set of patches to test. Doesn't bug 708813 just require this patch though?

(In reply to Mark Banner (:standard8) from comment #31) > (In reply to Honza Bambas (:mayhemer) from comment #30) > > Comment on attachment 590132 [details] [diff] [review] > > Part 1: Make SyncRunnableBase work without deadlocking when run on the main > Doesn't bug 708813 just require this patch though? Yes, that's right. This patch in particular should land only and only on beta. I'll copy the patch to that bug and r+ it. But I still don't think it is sufficient for this bug until the remaining patches get updated and I can test them as a complete set.

Honza, I wrote a script to help you review this, which I will attach next.

Here is the script to help review Part 2. That is a big patch but it is just moving code and fiddling with #includes and minor declarations that support the moved code.

Tryserver for Firefox seems to be broken right now. Thunderbird builds with these patches are currently building on cc-try: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=8f9793ec4860

Comment on attachment 590132 [details] [diff] [review] Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread [v2] Review of attachment 590132 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab for landing on m-c and r/m-a and actually as well as r/m-b (marked then in a different bug).

Comment on attachment 590524 [details] [diff] [review] Part 2: Move CertErrorRunnable, SSLServerCertVerificationResult, and HandleBadCertificate to SSLServerCertVerification.cpp Review of attachment 590524 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the script, Brian! Very helpful. r=honzab

Comment on attachment 590526 [details] [diff] [review] Part 3: Correct the documentation of how threading works Review of attachment 590526 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab

Comment on attachment 590529 [details] [diff] [review] Part 4: Split the creation of CertErrorRunnable from the dispatching of it Review of attachment 590529 [details] [diff] [review]: ----------------------------------------------------------------- This really is nice. r=honzab ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +358,3 @@ > } > > +// Returns null with the error code set if it does not create the "with the PR error code" ? @@ +508,5 @@ > } > > socketInfo->SetStatusErrorBits(*nssCert, collected_errors); > + nsSSLIOLayerHelpers::mHostsWithCertErrors->RememberCertHasError( > + socketInfo, socketInfo->SSLStatus(), SECFailure); You forgot to remove this call from SetStatusErrorBits if you still believe it is a good idea to call this separately. @@ +526,5 @@ > +// which may call nsHttpConnection::GetInterface() through > +// nsNSSSocketInfo::mCallbacks. nsHttpConnection::GetInterface must always > +// execute on the main thread, with the socket transport service thread > +// blocked. > +class CertErrorRunnableRunnable : public nsRunnable Interesting solution. @@ +540,5 @@ > + nsresult rv = mCertErrorRunnable->DispatchToMainThreadAndWait(); > + // The result must run on the socket transport thread, which we are already > + // on, so we can just run it directly, instead of dispatching it. > + if (NS_SUCCEEDED(rv)) { > + rv = mCertErrorRunnable->mResult->Run(); Maybe worth to also check on presence of mResult? Regardless of the MOZ_ASSERT in RunOnTargetThread. You have a similar check in AuthCertificateHook in Part 5 patch. Actually, there used to be a code to always create a result. But it was just a corner case insurance..

Comment on attachment 590530 [details] [diff] [review] Part 5: Part 5: Add back synchronous cert validation for Thunderbird Review of attachment 590530 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ -1119,5 @@ > // doing verification without checking signatures. > - NS_ASSERTION(checkSig, "AuthCertificateHook: checkSig unexpectedly false"); > - > - // PSM never causes libssl to call this function with PR_TRUE for isServer, > - // and many things in PSM assume that we are a client. Shouldn't be this removal in a different bug? @@ +1204,5 @@ > + error = PR_UNKNOWN_ERROR; > + } > + > + PR_SetError(error, 0); > + return SECFailure; Again, this is a code very much similar to the code in SSLServerCertVerificationJob::Run(). We should have a followup to try to stick this code in one place. For now, since it is that late, let's go with this, since it works well.

Addresses Honza's review comments.

Addresses Honza's review comments.

It seems people are overlooking that this bug also depends on bug 706955 being fixed. There is a patch in bug 706955 awaiting review. I have now just changed the reviewer.

Here are the tryserver results for mozilla-central, for these patches and a bunch more related ones: https://tbpl.mozilla.org/?tree=Try&rev=b63de8572b37

https://hg.mozilla.org/integration/mozilla-inbound/rev/9576aeb57bd4 https://hg.mozilla.org/integration/mozilla-inbound/rev/efae575a79e4 https://hg.mozilla.org/integration/mozilla-inbound/rev/e610972755d9 https://hg.mozilla.org/integration/mozilla-inbound/rev/0bcaac9fadf1 https://hg.mozilla.org/integration/mozilla-inbound/rev/d9eab22ce37a

https://hg.mozilla.org/mozilla-central/rev/9576aeb57bd4 https://hg.mozilla.org/mozilla-central/rev/efae575a79e4 https://hg.mozilla.org/mozilla-central/rev/e610972755d9 https://hg.mozilla.org/mozilla-central/rev/0bcaac9fadf1 https://hg.mozilla.org/mozilla-central/rev/d9eab22ce37a

Comment on attachment 590132 [details] [diff] [review] Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread [v2] [Approval Request Comment] This has landed on mozilla-central, it narrowly missed landing on mozilla-aurora by a few hours, when we were expecting it to get into mozilla-central. We could really do with this on mozilla-aurora so that LDAP is no longer broken there, and our testers (that include quite a few LDAP users) can use it again. Doing a one-off branch is not really viable for Thunderbird & SeaMonkey on mozilla-aurora. Regression caused by (bug #): Various PSM changes User impact if declined: LDAP over SSL hangs for Thunderbird & SeaMonkey Testing completed (on m-c, etc.): Landed on mozilla-central, now in nightlies, it is being released in Thunderbird 11 Beta via a special relbranches for each beta and then for final release. Risk to taking this patch (and alternatives if risky): Changes in this patch do affect Firefox, as said previously, this only missed the merge by a few hours. This will hopefully be somewhat mitigated by the extra testing received by the Betas. String changes made by this patch: None.

(In reply to Mark Banner (:standard8) from comment #51) > We could really do with this on mozilla-aurora so that LDAP is no longer > broken there, and our testers (that include quite a few LDAP users) can use > it again. Doing a one-off branch is not really viable for Thunderbird & > SeaMonkey on mozilla-aurora. I agree. We should land this on mozilla-aurora ASAP. > Regression caused by (bug #): Various PSM changes bug 674147 and maybe bug 675221

Did not intend to set target milestone.

Comment on attachment 590132 [details] [diff] [review] Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread [v2] [Triage Comment] Missed the merge by a few hours - approved for Aurora 12.

Transplanted to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/0bff753b0529 https://hg.mozilla.org/releases/mozilla-aurora/rev/38c4b6c2a63e https://hg.mozilla.org/releases/mozilla-aurora/rev/8e27df1d74b4 https://hg.mozilla.org/releases/mozilla-aurora/rev/dd0380ff4264 https://hg.mozilla.org/releases/mozilla-aurora/rev/e844448f2d1d

Thanks for fixing it for Firefox 12. I can no longer reproduce the freeze with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0a2) Gecko/20120226 Thunderbird/12.0a2 ID:20120226030023