Closed Bug 935769 Opened 6 years ago Closed 6 years ago

nsNSSCertList nor dnsNSSCertListEnumerator cleanup NSS resources appropiately

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: cviecco, Assigned: cviecco)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 5 obsolete files)

Both have ScopedCERTCertLists but do not guard agains NSS shutdown.
Assignee: nobody → cviecco
OS: Linux → All
Hardware: x86_64 → All
Attached patch add-nssshutdown-to-nsscertlist (obsolete) — Splinter Review
Blocks: 912155
Attachment #828328 - Flags: review?(dkeeler)
Comment on attachment 828328 [details] [diff] [review]
add-nssshutdown-to-nsscertlist

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

Don't we need shutdown checks in every member function that uses NSS data/functions as well?
Also, as a general comment, I think it's good to have braces even for one line conditionals.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1490,5 @@
>  
> +nsNSSCertList::~nsNSSCertList()
> +{
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown())

braces

@@ +1504,5 @@
> +}
> +
> +void nsNSSCertList::destructorSafeDestroyNSSReference()
> +{
> +  if (isAlreadyShutDown())

braces
Attachment #828328 - Flags: review?(dkeeler) → review-
Attachment #828328 - Attachment is obsolete: true
Comment on attachment 828343 [details] [diff] [review]
add-nssshutdown-to-nsscertlist (v2)

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

Thanks for the fast review. Comments addressed.
Attachment #828343 - Flags: review?(dkeeler)
Comment on attachment 828343 [details] [diff] [review]
add-nssshutdown-to-nsscertlist (v2)

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

Looks good. I just have a couple of questions, so r- for now.
What about nsNSSCertList::DupCertList? We may have to do the proofOfLock trick again.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1509,5 @@
> +  if (isAlreadyShutDown()) {
> +    return;
> +  }
> +  if (mCertList) {
> +    mCertList = nullptr;

I have a couple of questions about this:
1. I should know this already, but does setting a ScopedWhatever type to null call the destructor on the backing object?
2. If so, aren't we then calling NSS functions after it's been shut down?
3. If not, are we leaking memory? Or has it already been cleaned up by NSS shutting down?
Attachment #828343 - Flags: review?(dkeeler) → review-
(In reply to David Keeler (:keeler) from comment #6)
> Comment on attachment 828343 [details] [diff] [review]
> add-nssshutdown-to-nsscertlist (v2)
> 
> Review of attachment 828343 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. I just have a couple of questions, so r- for now.
> What about nsNSSCertList::DupCertList? We may have to do the proofOfLock
> trick again.

Sure.

> 
> ::: security/manager/ssl/src/nsNSSCertificate.cpp
> @@ +1509,5 @@
> > +  if (isAlreadyShutDown()) {
> > +    return;
> > +  }
> > +  if (mCertList) {
> > +    mCertList = nullptr;
> 
> I have a couple of questions about this:
> 1. I should know this already, but does setting a ScopedWhatever type to
> null call the destructor on the backing object?
I also had to look it up. The answer is yes:  http://mxr.mozilla.org/mozilla-central/source/mfbt/Scoped.h#133

> 2. If so, aren't we then calling NSS functions after it's been shut down?
No the nsNSSShutDownObject ensures that this is called before NSS is finally
shut down. Previously I was hitting precisely this, the scopedPTR was being
shut down after NSS and I was getting assertion failures (on the new tests)

> 3. If not, are we leaking memory? Or has it already been cleaned up by NSS
> shutting down?
Attached patch dupcert-lock-v1 (obsolete) — Splinter Review
dupcert is called only from the constructors. If we are ok with locks there then this is the patch, if not we can reduce the risk by:
1. making this a static function wihtin nssCertificate.cpp 
2. we could make it  a protected function.

or just ignore the potential issues. I have tested the lock approach and seems not to fail..
Attachment #828972 - Flags: feedback?(dkeeler)
Comment on attachment 828972 [details] [diff] [review]
dupcert-lock-v1

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

I think this is the right thing to do. Just one question (below).

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1476,5 @@
>  NS_IMPL_ISUPPORTS1(nsNSSCertList, nsIX509CertList)
>  
>  nsNSSCertList::nsNSSCertList(CERTCertList *certList, bool adopt)
>  {
> +  if (certList && adopt) {

I don't understand why this was moved - don't we need to do the shutdown check before anything else?
Attachment #828972 - Flags: feedback?(dkeeler) → feedback+
In the fast path (that is used but all but one call) there are no nss calls, just a pointer assignment. No need to add this to the critical section
Attachment #828343 - Attachment is obsolete: true
Attachment #828972 - Attachment is obsolete: true
Attachment #829389 - Flags: review?(dkeeler)
Comment on attachment 829389 [details] [diff] [review]
add-nssshutdown-to-nsscertlist (v3)

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

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1474,5 @@
>  }
>  
>  NS_IMPL_ISUPPORTS1(nsNSSCertList, nsIX509CertList)
>  
>  nsNSSCertList::nsNSSCertList(CERTCertList *certList, bool adopt)

just add the proofOfLock argument here. All the existing callers already hold the shutdown prevention lock when invoking this constructor.

@@ +1479,5 @@
>  {
> +  if (certList && adopt) {
> +    mCertList = certList;
> +    return;
> +  }

Generally, constructors should not give up silently trying to construct the object like this.

@@ +1497,5 @@
> +{
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    return;
> +  }

redundant with the check in destructorSafeDestroyNSSReference().

@@ +1607,5 @@
>  {
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    return nullptr;
> +  }

If NSS has already shut down then mCertList will be null already, so there's no need to do this check.

In addition, the ultimate caller of this is nsCertTree::GetCertsByTypeFromCache. That function is the one that should take the shutdown prevention lock. Since you cannot use the proofOfLock trick on this method, because it is declared in IDL, just add a comment to the IDL that says that it must be called only while holding the shutdown prevention lock.

@@ +1623,1 @@
>    nsCOMPtr<nsISimpleEnumerator> enumerator = new nsNSSCertListEnumerator(mCertList);

nsNSSCertListEnumerator should take a proofOfLock argument as well.

@@ +1634,5 @@
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    mCertList = nullptr;
> +    return;
> +  }

Same comments as for nsNSSCertList::nsNSSCertList. Don't half-construct objects.

@@ +1643,5 @@
> +{
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    return;
> +  }

redundant with the same check in destructorSafeDestroyNSSReference.
Attachment #829389 - Flags: review-
Comment on attachment 829389 [details] [diff] [review]
add-nssshutdown-to-nsscertlist (v3)

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

Clearing review since Brian did it instead. Just one comment to add.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1476,5 @@
>  NS_IMPL_ISUPPORTS1(nsNSSCertList, nsIX509CertList)
>  
>  nsNSSCertList::nsNSSCertList(CERTCertList *certList, bool adopt)
>  {
> +  if (certList && adopt) {

I still think this change needlessly complicates the logic of this constructor.
Attachment #829389 - Flags: review?(dkeeler)
Comment on attachment 829389 [details] [diff] [review]
add-nssshutdown-to-nsscertlist (v3)

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

Thanks for the review brian. But I have one issue: 

1. what to to on bad lock/unlocked call on the contructor: return early is what is being done on this same file. If you dont like this please state alternative.

and one question:

2. why is it better to have a comment on the idl and instead take the hit of double lock (on the few times it happens)?

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1474,5 @@
>  }
>  
>  NS_IMPL_ISUPPORTS1(nsNSSCertList, nsIX509CertList)
>  
>  nsNSSCertList::nsNSSCertList(CERTCertList *certList, bool adopt)

I can do a variation of this :as this is a constructor it requires default params and this I can pass a pointer to the lock.

@@ +1479,5 @@
>  {
> +  if (certList && adopt) {
> +    mCertList = certList;
> +    return;
> +  }

On this same file (http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCertificate.cpp#147) is it being done.

Can you tell me of an alternative when I cannot lock or the lock parameter is null?

@@ +1497,5 @@
> +{
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    return;
> +  }

ACK

@@ +1623,1 @@
>    nsCOMPtr<nsISimpleEnumerator> enumerator = new nsNSSCertListEnumerator(mCertList);

Ditto as from NSSCertList. What is the alternative for failire
Comment on attachment 829389 [details] [diff] [review]
add-nssshutdown-to-nsscertlist (v3)

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

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1474,5 @@
>  }
>  
>  NS_IMPL_ISUPPORTS1(nsNSSCertList, nsIX509CertList)
>  
>  nsNSSCertList::nsNSSCertList(CERTCertList *certList, bool adopt)

I don't understand why default parameters are useful or necessary here. We should be able to change the signature of nsNSSCertList::nsNSSCertList however we want, as long as we update all the callers, which seems easy to do.

@@ +1479,5 @@
>  {
> +  if (certList && adopt) {
> +    mCertList = certList;
> +    return;
> +  }

Make the lock parameter a reference (const nsNSSShutDownPreventionLock& /*proofOfLock*/) so it can't be null.

Since we require the caller to do the locking and ensure that NSS isn't shut down, we don't need to check that here, so the error case won't occur.

@@ +1623,1 @@
>    nsCOMPtr<nsISimpleEnumerator> enumerator = new nsNSSCertListEnumerator(mCertList);

Same answer as above. You don't need to check isAlreadyShutDown() because you can just assume that the caller already checked. That is part of the contract of the proofOfLock idiom.
(In reply to Camilo Viecco (:cviecco) from comment #14)
> 2. why is it better to have a comment on the idl and instead take the hit of
> double lock (on the few times it happens)?

If the function can be implemented correctly in the case where isAlreadyShutDown() returns true, then OK. it depends on whether the callers of GetRawCertList() can tolerate a null result.
Comment on attachment 829389 [details] [diff] [review]
add-nssshutdown-to-nsscertlist (v3)

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

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1474,5 @@
>  }
>  
>  NS_IMPL_ISUPPORTS1(nsNSSCertList, nsIX509CertList)
>  
>  nsNSSCertList::nsNSSCertList(CERTCertList *certList, bool adopt)

I misread errors from the compliter (and then read incorrect documentation) Yes this is possible.

@@ +1607,5 @@
>  {
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    return nullptr;
> +  }

Do you want and idel change to make it no-script? or the comment should be enough?
Attachment #829389 - Attachment is obsolete: true
Attachment #831119 - Flags: review?(brian)
solved your concers, want to delagate review to keeler?
Flags: needinfo?(brian)
Comment on attachment 831119 [details] [diff] [review]
add-nssshutdown-to-nsscertlist (v4)

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

Looks pretty good to me.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1475,5 @@
>  
>  NS_IMPL_ISUPPORTS1(nsNSSCertList, nsIX509CertList)
>  
> +nsNSSCertList::nsNSSCertList(const nsNSSShutDownPreventionLock &proofOfLock,
> +                             CERTCertList *certList, bool adopt)

NIT: The convention for all other uses of this pattern is for proofOfLock to be the last parameter. Please use that convention.

Note that all (both) callers of this constructor pass true for adopt, so you can remove the adopt parameter and simplify the logic below. (Please add a comment like /*transfers ownership*/ to certList.

@@ +1597,5 @@
>  void *
>  nsNSSCertList::GetRawCertList()
>  {
> +  // This function should only be called after adquiring a 
> +  // nsNSSShutDownPreventionLock

NIT: trailing whitespace.

@@ +1609,5 @@
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +  nsCOMPtr<nsISimpleEnumerator> enumerator = new nsNSSCertListEnumerator(mCertList, locker);

NIT: line length.
Attachment #831119 - Flags: review?(brian) → review-
Comment on attachment 831119 [details] [diff] [review]
add-nssshutdown-to-nsscertlist (v4)

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

Please address all the review comments in the previous comment before pushing this.
Attachment #831119 - Flags: review- → review+
clearing needinfo since I already did the reviews.
Flags: needinfo?(brian)
Keeping bsmith's r+
Attachment #831119 - Attachment is obsolete: true
Attachment #832971 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/0ed8a5d6395c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.