Closed Bug 967629 Opened 6 years ago Closed 6 years ago

don't release any member nsNSSShutDownObject in any destructorSafeDestroyNSSReference

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(1 file)

In bug 957368, I changed nsNSSCertCache::destructorSafeDestroyNSSReference() to release its mCertList (which is an nsIX509CertList). Turns out, if nsNSSCertCache is being cleaned up because of a call to evaporateAllNSSResources, releasing the nsIX509CertList will call its destructor, which calls its destructorSafeDestroyNSSReference, which will cause a concurrent delete operation on the hash table in the nsNSSShutDownList, which is bad.

Assertion failure: op == PL_DHASH_LOOKUP || table->recursionLevel == 0, at /home/keeler/mozilla-central/xpcom/glue/pldhash.cpp:492

#0  PL_DHashTableOperate (table=0x7fffd9295df8, key=0x7fffc677ba68, op=PL_DHASH_REMOVE) at /home/keeler/mozilla-central/xpcom/glue/pldhash.cpp:492
#1  0x00007ffff1665d72 in nsNSSShutDownList::forget (o=0x7fffc677ba68) at /home/keeler/mozilla-central/security/manager/ssl/src/nsNSSShutDown.cpp:91
#2  0x00007fffee3b7a59 in nsNSSShutDownObject::shutdown (this=0x7fffc677ba68, calledFrom=nsNSSShutDownObject::calledFromObject) at ../../../dist/include/nsNSSShutDown.h:260
#3  0x00007ffff165c498 in nsNSSCertList::~nsNSSCertList (this=0x7fffc677ba60) at /home/keeler/mozilla-central/security/manager/ssl/src/nsNSSCertificate.cpp:1563
#4  0x00007ffff165c3c9 in nsNSSCertList::~nsNSSCertList (this=0x7fffc677ba60) at /home/keeler/mozilla-central/security/manager/ssl/src/nsNSSCertificate.cpp:1557
#5  0x00007ffff165c205 in nsNSSCertList::Release (this=0x7fffc677ba60) at /home/keeler/mozilla-central/security/manager/ssl/src/nsNSSCertificate.cpp:1544
#6  0x00007ffff16033f6 in nsCOMPtr<nsIX509CertList>::assign_assuming_AddRef (this=0x7fffc734a4b8, newPtr=0x0) at ../../../../dist/include/nsCOMPtr.h:502
#7  0x00007ffff16031c6 in nsCOMPtr<nsIX509CertList>::assign_with_AddRef (this=0x7fffc734a4b8, rawPtr=0x0) at ../../../../dist/include/nsCOMPtr.h:1194
#8  0x00007ffff1602f1f in nsCOMPtr<nsIX509CertList>::operator= (this=0x7fffc734a4b8, rhs=0x0) at ../../../../dist/include/nsCOMPtr.h:659
#9  0x00007ffff164e3b6 in nsNSSCertCache::destructorSafeDestroyNSSReference (this=0x7fffc734a480) at /home/keeler/mozilla-central/security/manager/ssl/src/nsNSSCertCache.cpp:38
#10 0x00007ffff164e3d5 in nsNSSCertCache::virtualDestroyNSSReference (this=0x7fffc734a480) at /home/keeler/mozilla-central/security/manager/ssl/src/nsNSSCertCache.cpp:33
#11 0x00007ffff164e3fc in non-virtual thunk to nsNSSCertCache::virtualDestroyNSSReference() (this=0x7fffc734a488) at Unified_cpp_security_manager_ssl_src1.cpp:34
#12 0x00007fffee3b7a74 in nsNSSShutDownObject::shutdown (this=0x7fffc734a488, calledFrom=nsNSSShutDownObject::calledFromList) at ../../../dist/include/nsNSSShutDown.h:263
#13 0x00007ffff16664f2 in nsNSSShutDownList::evaporateAllNSSResourcesHelper (table=0x7fffd9295df8, hdr=0x7fffc6130f30, number=0, arg=0x0) at /home/keeler/mozilla-central/security/manager/ssl/src/nsNSSShutDown.cpp:219
#14 0x00007fffee1e49ed in PL_DHashTableEnumerate (table=0x7fffd9295df8, etor=0x7ffff1666490 <nsNSSShutDownList::evaporateAllNSSResourcesHelper(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*)>, arg=0x0) at /home/keeler/mozilla-central/xpcom/glue/pldhash.cpp:632
#15 0x00007ffff166632a in nsNSSShutDownList::evaporateAllNSSResources (this=0x7fffd9295de0) at /home/keeler/mozilla-central/security/manager/ssl/src/nsNSSShutDown.cpp:205
#16 0x00007ffff1605ab7 in nsNSSComponent::ShutdownNSS (this=0x7fffd93b7d80) at /home/keeler/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:1239
#17 0x00007ffff160b336 in nsNSSComponent::DoProfileBeforeChange (this=0x7fffd93b7d80, aSubject=0x0) at /home/keeler/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:1776
#18 0x00007ffff160aaa9 in nsNSSComponent::Observe (this=0x7fffd93b7d80, aSubject=0x0, aTopic=0x7ffff2f9bf88 "profile-before-change", someData=0x7ffff34f7960 u"shutdown-persist") at /home/keeler/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:1506
#19 0x00007ffff160b41f in non-virtual thunk to nsNSSComponent::Observe(nsISupports*, char const*, char16_t const*) (this=0x7fffd93b7d98, aSubject=0x0, aTopic=0x7ffff2f9bf88 "profile-before-change", someData=0x7ffff34f7960 u"shutdown-persist") at /home/keeler/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:1620
#20 0x00007fffee2638e9 in nsObserverList::NotifyObservers (this=0x7fffcfc17270, aSubject=0x0, aTopic=0x7ffff2f9bf88 "profile-before-change", someData=0x7ffff34f7960 u"shutdown-persist") at /home/keeler/mozilla-central/xpcom/ds/nsObserverList.cpp:96
#21 0x00007fffee2655d1 in nsObserverService::NotifyObservers (this=0x7fffe4214da0, aSubject=0x0, aTopic=0x7ffff2f9bf88 "profile-before-change", someData=0x7ffff34f7960 u"shutdown-persist") at /home/keeler/mozilla-central/xpcom/ds/nsObserverService.cpp:302
#22 0x00007ffff16b2ef6 in nsXREDirProvider::DoShutdown (this=0x7fffffffc038) at /home/keeler/mozilla-central/toolkit/xre/nsXREDirProvider.cpp:871
#23 0x00007ffff1694fe7 in ScopedXPCOMStartup::~ScopedXPCOMStartup (this=0x7ffff6a82cf0) at /home/keeler/mozilla-central/toolkit/xre/nsAppRunner.cpp:1158
#24 0x00007ffff169e797 in XREMain::XRE_main (this=0x7fffffffbff8, argc=5, argv=0x7fffffffd5c8, aAppData=0x7fffffffc298) at /home/keeler/mozilla-central/toolkit/xre/nsAppRunner.cpp:4183
#25 0x00007ffff169ee6f in XRE_main (argc=5, argv=0x7fffffffd5c8, aAppData=0x7fffffffc298, aFlags=0) at /home/keeler/mozilla-central/toolkit/xre/nsAppRunner.cpp:4368
#26 0x0000000000404b99 in do_main (argc=5, argv=0x7fffffffd5c8, xreDirectory=0x7ffff6a2c600) at /home/keeler/mozilla-central/browser/app/nsBrowserApp.cpp:280
#27 0x000000000040426c in main (argc=5, argv=0x7fffffffd5c8) at /home/keeler/mozilla-central/browser/app/nsBrowserApp.cpp:648
The test in bug 860076 exposes this fairly regularly.
Blocks: 860076
David, do you think this could be linked to 963317?
Could be. If it stops after I land a fix, I'd imagine this was the problem.
(In reply to David Keeler (:keeler) from comment #3)
> Could be. If it stops after I land a fix, I'd imagine this was the problem.

thanks! 865314 is blocked on that (it makes the crash in jstreftest1 on android happen a lot) - so I'd be happy to try out version on try.
Attached patch patchSplinter Review
I reverted the bad release I added as well as another I found.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8370206 - Flags: review?(cviecco)
Comment on attachment 8370206 [details] [diff] [review]
patch

Review of attachment 8370206 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/identity/IdentityCryptoService.cpp
@@ -155,5 @@
>    }
>  
>    void destructorSafeDestroyNSSReference()
>    {
> -    mKeyPair = nullptr;

This existed before your changes to bug 957368. Why change it now? (if so is not safer to revert 957368?)
Attachment #8370206 - Flags: review?(cviecco)
Flags: needinfo?(dkeeler)
mKeyPair is a nsCOMPtr<KeyPair>, and KeyPair is an nsNSSShutDownObject. Turns out, it's not safe for one nsNSSShutDownObject to cause another one to be deconstructed when running its destructorSafeDestroyNSSReference. So, this was always wrong, but I think this is rarely used, so no one noticed. The correct thing to do is for mKeyPair to be released implicitly when KeyGenRunnable is deconstructed (this will call its destructor which will check for NSS shutdown, etc.).
There are other changes in the patch from bug 957368 that are correct (as far as I know, of course), so we shouldn't revert it wholesale.
Flags: needinfo?(dkeeler)
Attachment #8370206 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/adc96670dbb8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8370206 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 957368
User impact if declined: assertion failures on shutdown in debug builds, potential for crashing on shutdown in non-debug builds
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none
Attachment #8370206 - Flags: approval-mozilla-aurora?
(In reply to David Keeler (:keeler) from comment #8)
> mKeyPair is a nsCOMPtr<KeyPair>, and KeyPair is an nsNSSShutDownObject.
> Turns out, it's not safe for one nsNSSShutDownObject to cause another one to
> be deconstructed when running its destructorSafeDestroyNSSReference.

That's...not good. Could you please start an email thread explaining why?
Attachment #8370206 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed with the patch for bug 860076.
https://hg.mozilla.org/releases/mozilla-aurora/rev/bd26e83cefca

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #12)
> (In reply to David Keeler (:keeler) from comment #8)
> > mKeyPair is a nsCOMPtr<KeyPair>, and KeyPair is an nsNSSShutDownObject.
> > Turns out, it's not safe for one nsNSSShutDownObject to cause another one to
> > be deconstructed when running its destructorSafeDestroyNSSReference.
> 
> That's...not good. Could you please start an email thread explaining why?

Yes - I'll try to get to it today. What group? security-engineering? dev-platform? (is there a psm-specific one?)
You need to log in before you can comment on or make changes to this bug.