Closed Bug 767241 Opened 7 years ago Closed 7 years ago

Create scoped pointer types for NSS types

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(3 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #767240 +++

This patch adds types such as ScopedSECKEYPrivateKey, ScopedCERTCertificate, etc. to a header in PSM that act like smart pointers for commonly-used NSS types. The advantages over NSSCleanupAutoPtrClass are:

1. These are more consistent with the other Scoped* types in Gecko.
2. These are defined in a single header, whereas the currently-used pattern for NSSCleanupAutoPtrClass has us declare the same Cleanup types over and over again in each .cpp file.
3. ScopedNSSTypes.h is exported so code outside of PSM can use it.
4. NSSCleanupAutoPtrClass requires us to write code like this:
      #include "nsNSSCleaner.h"
      NSSCleanupAutoPtrClass(Type, TYPE_DestroyType)

      Type * pointer;
      TypeCleaner pointerCleaner(pointer);
      pointer = TYPE_CreateType(....);

   whereas with this header, we can write just:

      #include "ScopedNSSTypes.h"

      ScopedType pointer(TYPE_CreateType(....));

Assuming this gets r+d, I will file a follow-up bug for converting the current uses of NSSCleanupAutoPtrClass to Scoped*.
Attachment #635574 - Attachment is obsolete: true
Attachment #665219 - Flags: review?(honzab.moz)
Note that these types are exported from PSM so that other code (in particular, the WebRTC code and BrowserID code) can use them. I did not change the void* use of NSSCleanupAutoPtr and I did not change any uses of NSSCleanupAutoPtr_WithParam. I would like to do that in another bug.
Attachment #665220 - Flags: review?(honzab.moz)
The SSLtunnel code has a couple hand-rolled wrappers like these:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/ssltunnel/ssltunnel.cpp#207

Should be trivial to replace those with these new classes. Very nice!
Attachment #665219 - Flags: review?(honzab.moz) → review+
Comment on attachment 665220 [details] [diff] [review]
Part 2: Replace almost all uses of NSSCleanupAutoPtr

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

r- for serious omissions.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +705,5 @@
>      NS_ASSERTION(intSlot,"Couldn't get the internal slot");
>      
>      if (!PK11_DoesMechanism(intSlot, mechanism)) {
>        // Set to null, and the subsequent code will not attempt to use it.
>        PK11_FreeSlot(intSlot);

Update this code to just |intSlot = nullptr;| otherwise you have double free.

@@ +830,5 @@
>   
>    //If we generated the key pair on the internal slot because the
>    // keys were going to be escrowed, move the keys over right now.
>    if (mustMoveKey) {
> +    ScopedSECKEYPrivateKey newPrivKey(PK11_LoadPrivKey(slot, 

trailing white space

@@ -848,5 @@
> -    // after the requests are made.  This call only gives up
> -    // our reference to the key object and does not actually 
> -    // physically remove it from the card itself.
> -    // The actual delete calls are being made in the destructors
> -    // of the cleaner helper instances.

For curiosity, why are you removing this comment?

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +1635,5 @@
>        nickname = tmp;
>        PR_smprintf_free(tmp);
>      }
>  
> +    ScopedCERTCertificate dummycert;

At [1] dummycert is being destroyed.  You have to just assign dummycert = nullptr with your new code or you have a double deref.

[1] http://hg.mozilla.org/mozilla-central/annotate/85df971e0db1/security/manager/ssl/src/nsNSSCertificateDB.cpp#l1674

@@ +1695,2 @@
>    CERTCertDBHandle *certdb = CERT_GetDefaultCertDB();
> +  ScopedCERTCertificate tmpCert(CERT_FindCertByDERCert(certdb, &der));

Here similar issue at [2], just remove call to destroy function probably.

[2] http://hg.mozilla.org/mozilla-central/annotate/85df971e0db1/security/manager/ssl/src/nsNSSCertificateDB.cpp#l1727

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +1958,5 @@
>      if (CERT_LIST_END(node, certList)) {
>        goto noCert;
>      }
>  
> +    ScopedCERTCertificate low_prio_nonrep_cert;

Please check the logic at [3].  You may need to add forget() or change the pointer mishmash here.

[3] http://hg.mozilla.org/mozilla-central/annotate/85df971e0db1/security/manager/ssl/src/nsNSSIOLayer.cpp#l2002
Attachment #665220 - Flags: review?(honzab.moz) → review-
Comment on attachment 665219 [details] [diff] [review]
Part 1: Create scoped pointer types for NSS types

https://hg.mozilla.org/mozilla-central/rev/ddd4e4a5f337

Checked in so that WebRTC can start using these types.
Attachment #665219 - Flags: checkin+
Target Milestone: --- → mozilla18
Honza, thanks for your careful review. In order to ensure there are no more double-free problems, I removed all the CERT_Destroy* calls from the functions/files I modified.
Attachment #665220 - Attachment is obsolete: true
Attachment #683899 - Flags: review?(honzab.moz)
Here is a try run that includes these changes amongst many others. Ignore the xpcshell test failures; they are intentional in this try run and unrelated to this bug.
Here is a try run that includes these changes amongst many others. Ignore the xpcshell test failures; they are intentional in this try run and unrelated to this bug: https://tbpl.mozilla.org/?tree=Try&rev=af50fdec15a6
Attachment #683900 - Flags: review?(ted) → review+
Comment on attachment 683899 [details] [diff] [review]
Part 2: Replace almost all uses of NSSCleanupAutoPtr and most manual uses of CERT_Destroy* ad friends

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

Not that deep review as the last time (too time consuming).  I checked the major issues has been fixed.

r=honzab.

::: security/manager/ssl/src/nsNSSCallbacks.h
@@ +16,5 @@
>  #include "mozilla/Mutex.h"
>  #include "mozilla/Attributes.h"
> +#include "nsString.h"
> +
> +class nsILoadGroup;

Changes from a different bug?
Attachment #683899 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c966b16e4fb5

(In reply to Honza Bambas (:mayhemer) from comment #11)
> ::: security/manager/ssl/src/nsNSSCallbacks.h
> @@ +16,5 @@
> >  #include "mozilla/Mutex.h"
> >  #include "mozilla/Attributes.h"
> > +#include "nsString.h"
> > +
> > +class nsILoadGroup;
> 
> Changes from a different bug?

This is needed due to the removal of an #include from an included header file. Thanks for the review!
Target Milestone: mozilla18 → mozilla20
Summary: Create scoped pointer types for NSS types, so we don't have to use keep using NSSCleanupAutoPtrClass over and oer → Create scoped pointer types for NSS types
You need to log in before you can comment on or make changes to this bug.