619198 - nsStrictTransportSecurityService can be created off main thread and uses non-threadsafe nsPermissionManager

Status: RESOLVED WORKSFORME

(Core :: Security: PSM, defect)

Description

[Josh Matthews [:jdm]]

Starting Fennec on a restricted wireless network currently brings up an SSL cert error dialog which triggers this assertion:

#0  RealBreak () at /home/t_mattjo/src/firefox/mobilebase/xpcom/base/nsDebugImpl.cpp:407
#1  0x0258d684 in Break (aMsg=
    0xb00fe48c "###!!! ASSERTION: nsPermissionManager not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /home/t_mattjo/src/firefox/mobilebase/extensions/cookie/nsPermissionManager.cpp, line"...) at /home/t_mattjo/src/firefox/mobilebase/xpcom/base/nsDebugImpl.cpp:503
#2  0x0258d649 in NS_DebugBreak_P (aSeverity=1, aStr=0x2f5bb98 "nsPermissionManager not thread-safe", aExpr=0x2f5bb64 "_mOwningThread.GetThread() == PR_GetCurrentThread()", aFile=
    0x2f5ba7c "/home/t_mattjo/src/firefox/mobilebase/extensions/cookie/nsPermissionManager.cpp", aLine=171) at /home/t_mattjo/src/firefox/mobilebase/xpcom/base/nsDebugImpl.cpp:371
#3  0x0209e29b in nsPermissionManager::AddRef (this=0xb31b0be0) at /home/t_mattjo/src/firefox/mobilebase/extensions/cookie/nsPermissionManager.cpp:171
#4  0x019c1e77 in nsCOMPtr<nsISupports>::nsCOMPtr (this=0xb00fe904, aSmartPtr=...) at ../../dist/include/nsCOMPtr.h:906
#5  0x02572e11 in nsComponentManagerImpl::GetServiceByContractID (this=0xb7dc3220, aContractID=0x2f33a94 "@mozilla.org/permissionmanager;1", aIID=..., result=0xb00fe9fc)
    at /home/t_mattjo/src/firefox/mobilebase/xpcom/components/nsComponentManager.cpp:1613
#6  0x025063a2 in CallGetService (aContractID=0x2f33a94 "@mozilla.org/permissionmanager;1", aIID=..., aResult=0xb00fe9fc) at nsComponentManagerUtils.cpp:94
#7  0x025068d6 in nsGetServiceByContractIDWithError::operator() (this=0xb00fea60, aIID=..., aInstancePtr=0xb00fe9fc) at nsComponentManagerUtils.cpp:288
#8  0x01fc4f52 in nsCOMPtr<nsIPermissionManager>::assign_from_gs_contractid_with_error (this=0xae17369c, gs=..., aIID=...) at ../../../../dist/include/nsCOMPtr.h:1262
#9  0x01fc4e30 in nsCOMPtr<nsIPermissionManager>::operator= (this=0xae17369c, rhs=...) at ../../../../dist/include/nsCOMPtr.h:721
#10 0x01fc3a9a in nsStrictTransportSecurityService::Init (this=0xae173690) at /home/t_mattjo/src/firefox/mobilebase/security/manager/boot/src/nsStrictTransportSecurityService.cpp:80
#11 0x01fc212a in nsStrictTransportSecurityServiceConstructor (aOuter=0x0, aIID=..., aResult=0xb00feb38) at /home/t_mattjo/src/firefox/mobilebase/security/manager/boot/src/nsBOOTModule.cpp:49
#12 0x02514d28 in mozilla::GenericFactory::CreateInstance (this=0xae173680, aOuter=0x0, aIID=..., aResult=0xb00feb38) at GenericFactory.cpp:48
#13 0x0257244f in nsComponentManagerImpl::CreateInstanceByContractID (this=0xb7dc3220, aContractID=0x2f3c3a9 "@mozilla.org/stsservice;1", aDelegate=0x0, aIID=..., aResult=0xb00feb38)
    at /home/t_mattjo/src/firefox/mobilebase/xpcom/components/nsComponentManager.cpp:1315
#14 0x025730c2 in nsComponentManagerImpl::GetServiceByContractID (this=0xb7dc3220, aContractID=0x2f3c3a9 "@mozilla.org/stsservice;1", aIID=..., result=0xb00fec2c)
    at /home/t_mattjo/src/firefox/mobilebase/xpcom/components/nsComponentManager.cpp:1676
#15 0x025063a2 in CallGetService (aContractID=0x2f3c3a9 "@mozilla.org/stsservice;1", aIID=..., aResult=0xb00fec2c) at nsComponentManagerUtils.cpp:94
#16 0x02506890 in nsGetServiceByContractID::operator() (this=0xb00fec44, aIID=..., aInstancePtr=0xb00fec2c) at nsComponentManagerUtils.cpp:278
#17 0x0110cad4 in nsCOMPtr<nsIStrictTransportSecurityService>::assign_from_gs_contractid (this=0xb00fed94, gs=..., aIID=...) at ../../../dist/include/nsCOMPtr.h:1252
#18 0x01feb704 in nsCOMPtr<nsIStrictTransportSecurityService>::nsCOMPtr (this=0xb00fed94, gs=...) at ../../../../dist/include/nsCOMPtr.h:627
#19 0x01fe9c25 in nsNSSBadCertHandler (arg=0xb058f710, sslSocket=0xae1bf0e0) at /home/t_mattjo/src/firefox/mobilebase/security/manager/ssl/src/nsNSSIOLayer.cpp:3499
#20 0x0036aaa6 in ssl3_HandleCertificate (ss=0xae162000, b=
    0xae1a0388 "\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245"..., length=0)
    at ssl3con.c:7911
#21 0x0036c49a in ssl3_HandleHandshakeMessage (ss=0xae162000, b=0xae1a0004 "", length=900) at ssl3con.c:8603
#22 0x0036c928 in ssl3_HandleHandshake (ss=0xae162000, origBuf=0xae162250) at ssl3con.c:8727
#23 0x0036d40a in ssl3_HandleRecord (ss=0xae162000, cText=0xb00ff08c, databuf=0xae162250) at ssl3con.c:9066
#24 0x0036e527 in ssl3_GatherCompleteHandshake (ss=0xae162000, flags=0) at ssl3gthr.c:209
#25 0x0037117b in ssl_GatherRecord1stHandshake (ss=0xae162000) at sslcon.c:1258
#26 0x0037c824 in ssl_Do1stHandshake (ss=0xae162000) at sslsecur.c:151
#27 0x0037ea68 in ssl_SecureSend (ss=0xae162000, buf=
    0xae110000 "GET /en-US/mobile/api/1.5/list/featured/all/4/Linux/4.0b3pre HTTP/1.1\r\nHost: services.addons.mozilla.org\r\nUser-Agent: Mozilla/5.0 (X11; Linux i686; rv:2.0b8pre) Gecko/20101213 Firefox/4.0b8pre Fennec/"..., len=631, flags=0) at sslsecur.c:1213
#28 0x0037ec22 in ssl_SecureWrite (ss=0xae162000, buf=
    0xae110000 "GET /en-US/mobile/api/1.5/list/featured/all/4/Linux/4.0b3pre HTTP/1.1\r\nHost: services.addons.mozilla.org\r\nUser-Agent: Mozilla/5.0 (X11; Linux i686; rv:2.0b8pre) Gecko/20101213 Firefox/4.0b8pre Fennec/"..., len=631) at sslsecur.c:1258
#29 0x0038628f in ssl_Write (fd=0xae1bf0e0, buf=0xae110000, len=631) at sslsock.c:1652
#30 0x01fccf3c in nsSSLThread::Run (this=0xb01705e0) at /home/t_mattjo/src/firefox/mobilebase/security/manager/ssl/src/nsSSLThread.cpp:1045
#31 0x01fcb26b in nsPSMBackgroundThread::nsThreadRunner (arg=0xb01705e0) at /home/t_mattjo/src/firefox/mobilebase/security/manager/ssl/src/nsPSMBackgroundThread.cpp:44
#32 0x00182797 in _pt_root (arg=0xb037fa80) at /home/t_mattjo/src/firefox/mobilebase/nsprpub/pr/src/pthreads/ptthread.c:187
#33 0x008ba925 in start_thread () from /lib/libpthread.so.0
#34 0x007e407e in clone () from /lib/libc.so.6
(gdb) fr 10
#10 0x01fc3a9a in nsStrictTransportSecurityService::Init (this=0xae173690) at /home/t_mattjo/src/firefox/mobilebase/security/manager/boot/src/nsStrictTransportSecurityService.cpp:80
80	   mPermMgr = do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);
(gdb) list
75	nsresult
76	nsStrictTransportSecurityService::Init()
77	{
78	   nsresult rv;
79	
80	   mPermMgr = do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);
81	   NS_ENSURE_SUCCESS(rv, rv);
82	
83	   return NS_OK;
84	}

Comment 1

[[e-mail]]

Scary!

Comment 2

[Shawn Wilsher :sdwilsh]

I don't think the solution here is to make the permission manager threadsafe.  Looks like the real bug here is that PSM is accessing something off of the main thread that it shouldn't be.

Comment 3

[Doug Turner (:dougt)]

blocking - we want to figure out what the underlying problem is.

Comment 4

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/search?string=ns_stsservice&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central shows that anyone using the STS either is on the main thread or, in the case of nsNSSBadCertHandler, creates a proxy object that ensures that any uses of the permission manager inside of the STS is main-thread only.  However, if nsNSSBadCertHandler is the first user of the STS, it'll create it on the SSL thread and cause the assertion in this bug.

Comment 5

[Kai Engert (:KaiE:)]

Assigning to Sid, who implemented STS.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

(this affects Thunderbird (on linux) too)

(In reply to comment #5)
> Assigning to Sid, who implemented STS.

Kai, did you mean to actually assign the bug to someone or add someone to the cc?

Comment 7

[Kai Engert (:KaiE:)]

> Kai, did you mean to actually assign the bug to someone?

Yes, done now, I must have missed this. Thanks for noticing.

Assigning to Sid, who implemented STS.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

This patch attempts to initialize nsStrictTransportSecurityService from inside nsNSSComponent::Init (which given bug 615328, should only be on the main thread).  Any future calls to do_GetService(NS_STSSERVICE_CONTRACTID) should result in the already-created instance on the main thread.

I also tossed a check in nsStrictTransportSecurityService::Init() to fail if the instance is Init()ed on the wrong thread.

jdm: can you check to see if this works for you?  Until there's a patch for bug 615328, nsStrictTransportSecurityService::Init() should fail and return an error when it's created on the wrong thread.

Comment 9

[Josh Matthews [:jdm]]

I don't believe I have any way of testing this, as the bug in fennec was fixed that triggered the SSL cert failure dialog.

Comment 10

[Kai Engert (:KaiE:)]

Comment on attachment 501754 [details] [diff] [review]
proposed patch

I don't know if XPCOM will keep a service alive after creation.
Could XPCOM release the service immediately after nsNSSComponent::Init is done?

Maybe you should use a member pointer nsNSSComponent::mStrictTranspSecServ
to ensure the service instance will be kept alive.

This patch is correct, r=kaie
but consider to update the patch and ask for r? again.

Comment 11

[Kai Engert (:KaiE:)]

Note bug 619201 is a similar bug where the same question is being asked.

Comment 12

[Benjamin Smedberg]

Once a service is created *successfully*, XPCOM keeps it alive until shutdown.

Comment 13

[Kai Engert (:KaiE:)]

Thanks for your clarification, it means the patch is sufficient.

However, Sid, if you believe creation of PSM should strictly require STS, and if STS init fails, then all security/SSL should fail, too, you could enhance your patch in a way similar to bug 619201.

Comment 14

[Stuart Parmenter]

Can we get this landed?

Comment 15

[Kai Engert (:KaiE:)]

We apparently have many places where PSM accesses services from secondary threads, this isn't new code.

A recent change had introduced assertions/warnings for such scenarios, and it caused drivers to mark this bug as blocking+.

However, that constraining code has now been backed out - because it apparently had caused too many problems.

So, at this time, it might be unnecessary to land this code.
In addition, I had proposed to improve the current patch, see comment 13.

I think it's up to Sid whether he wants to land this code, or not.

Maybe it's better to wait for a better plan? The similar bug 619201 has not been landed either, it has actually been rejected by beltzner.

Comment 16

[Josh Matthews [:jdm]]

(In reply to comment #15)
> A recent change had introduced assertions/warnings for such scenarios, and it
> caused drivers to mark this bug as blocking+.
> 
> However, that constraining code has now been backed out - because it apparently
> had caused too many problems.

Isn't this referring to the use of the pref service off the main thread?  That's an orthogonal issue to this one.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

not waiting for this

Comment 18

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

Comment on attachment 501754 [details] [diff] [review]
proposed patch

This patch is not correct. It has the same problem as bug 650858.

Comment 19

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

This will be fixed when the patch in bug 679036 lands.

Comment 20

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

Works for me after the patch for bug 679140 was checked in.

I verified this by going to https://www.cacert.org immediately after opening the browser in a debug build. Before the patch for bug 679140, I would get the assertions about the permission service being used off the main thread. After the patch for bug bug 679140, I do not get any such assertions.

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can we put an assertion in nsStrictTransportSecurityService's ctor that asserts it's being constructed on the main thread so that we don't have to rely on the permissions manager yelling at us to see this problem?

Comment 22

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #21)
> Can we put an assertion in nsStrictTransportSecurityService's ctor that
> asserts it's being constructed on the main thread so that we don't have to
> rely on the permissions manager yelling at us to see this problem?

For posterity, this was addressed in bug 775370.