Closed Bug 1252722 Opened 4 years ago Closed 4 years ago

Address misc issues regarding nsPKCS11Slot and nsPKCS11ModuleDB

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(5 files)

Basically, fix all the things mentioned in Bug 1251801 comment 0, but for nsPKCS11Slot and nsPKCS11ModuleDB.
Summary: Fully implement nsNSSShutDownObject and obviate manual NSS resource management in nsPKCS11Slot and nsPKCS11ModuleDB → Addess misc issues regarding nsPKCS11Slot and nsPKCS11ModuleDB
Depends on: 1259149
Summary: Addess misc issues regarding nsPKCS11Slot and nsPKCS11ModuleDB → Address misc issues regarding nsPKCS11Slot and nsPKCS11ModuleDB
Whiteboard: [psm-assigned]
Review commit: https://reviewboard.mozilla.org/r/43407/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43407/
Attachment #8736577 - Flags: review?(dkeeler)
Attachment #8736578 - Flags: review?(dkeeler)
Attachment #8736579 - Flags: review?(dkeeler)
Attachment #8736580 - Flags: review?(dkeeler)
Attachment #8736581 - Flags: review?(dkeeler)
Comment on attachment 8736577 [details]
MozReview Request: Bug 1252722 - Fully implement nsNSSShutDownObject everywhere. r=keeler

https://reviewboard.mozilla.org/r/43407/#review40149

Cool.
Comment on attachment 8736578 [details]
MozReview Request: Bug 1252722 - Use smart pointers for NSS resources. r=keeler

https://reviewboard.mozilla.org/r/43409/#review40157
Attachment #8736578 - Flags: review?(dkeeler) → review+
Comment on attachment 8736579 [details]
MozReview Request: Bug 1252722 - Ensure arguments of all public methods are checked. r=keeler

https://reviewboard.mozilla.org/r/43411/#review40159

Looks good.

::: security/manager/ssl/nsPKCS11Slot.cpp:457
(Diff revision 1)
>  
> -NS_IMETHODIMP 
> -nsPKCS11ModuleDB::FindModuleByName(const char16_t *aName,
> -                                   nsIPKCS11Module **_retval)
> +NS_IMETHODIMP
> +nsPKCS11ModuleDB::FindModuleByName(const char16_t* aName,
> +                                   nsIPKCS11Module** _retval)
>  {
> +  // Note: It's OK for |aName| to be null.

This seems a little strange. In any case, should we add a test?
Attachment #8736579 - Flags: review?(dkeeler) → review+
Comment on attachment 8736580 [details]
MozReview Request: Bug 1252722 - Improve handling of PK11_* function error codes. r=keeler

https://reviewboard.mozilla.org/r/43413/#review40161
Attachment #8736580 - Flags: review?(dkeeler) → review+
Comment on attachment 8736581 [details]
MozReview Request: Bug 1252722 - Add additional tests. r=keeler

https://reviewboard.mozilla.org/r/43415/#review40163

Looks good. Just a few ideas for some more things we can check.

::: security/manager/ssl/tests/unit/test_pkcs11_module.js:114
(Diff revision 1)
> +  let strBundleSvc = Cc["@mozilla.org/intl/stringbundle;1"]
> +                       .getService(Ci.nsIStringBundleService);
> +  let bundle =
> +    strBundleSvc.createBundle("chrome://pipnss/locale/pipnss.properties");
> +  let internalTokenName = bundle.GetStringFromName("PrivateTokenDescription");
> +  notEqual(gModuleDB.findSlotByName(internalTokenName), null,

Should we check that this has the expected properties? (And that it's actually an nsIPKCS11Slot?)

::: security/manager/ssl/tests/unit/test_pkcs11_slot.js:45
(Diff revision 1)
>    //       The test module inserts and removes the test token in a tight loop,
>    //       so the result might not be deterministic.
> +
> +  // Note: testSlot.tokenName isn't tested for the same reason testSlot.status
> +  //       isn't.
> +  notEqual(testSlot.getToken(), null, "getToken() should succeed");

Should we spot-check that this has some expected properties?
Attachment #8736581 - Flags: review?(dkeeler) → review+
https://reviewboard.mozilla.org/r/43411/#review40159

> This seems a little strange. In any case, should we add a test?

As discussed on IRC: the behaviour is strange, but it's existing behaviour and there are several other places that do the same thing.
Adding tests for these cases probably won't be too difficult, but don't seem like a big win at the moment.
https://reviewboard.mozilla.org/r/43415/#review40163

> Should we check that this has the expected properties? (And that it's actually an nsIPKCS11Slot?)

Yes, these sound like good things to test. I'll add them.

> Should we spot-check that this has some expected properties?

Sounds like a good idea.
Comment on attachment 8736577 [details]
MozReview Request: Bug 1252722 - Fully implement nsNSSShutDownObject everywhere. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43407/diff/1-2/
Attachment #8736577 - Attachment description: MozReview Request: Bug 1252722 - Fully implement nsNSSShutDownObject everywhere. → MozReview Request: Bug 1252722 - Fully implement nsNSSShutDownObject everywhere. r=keeler
Attachment #8736578 - Attachment description: MozReview Request: Bug 1252722 - Use smart pointers for NSS resources. → MozReview Request: Bug 1252722 - Use smart pointers for NSS resources. r=keeler
Attachment #8736579 - Attachment description: MozReview Request: Bug 1252722 - Ensure arguments of all public methods are checked. → MozReview Request: Bug 1252722 - Ensure arguments of all public methods are checked. r=keeler
Attachment #8736580 - Attachment description: MozReview Request: Bug 1252722 - Improve handling of PK11_* function error codes. → MozReview Request: Bug 1252722 - Improve handling of PK11_* function error codes. r=keeler
Attachment #8736581 - Attachment description: MozReview Request: Bug 1252722 - Add additional tests. → MozReview Request: Bug 1252722 - Add additional tests. r=keeler
Comment on attachment 8736578 [details]
MozReview Request: Bug 1252722 - Use smart pointers for NSS resources. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43409/diff/1-2/
Comment on attachment 8736579 [details]
MozReview Request: Bug 1252722 - Ensure arguments of all public methods are checked. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43411/diff/1-2/
Comment on attachment 8736580 [details]
MozReview Request: Bug 1252722 - Improve handling of PK11_* function error codes. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43413/diff/1-2/
Comment on attachment 8736581 [details]
MozReview Request: Bug 1252722 - Add additional tests. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43415/diff/1-2/
You need to log in before you can comment on or make changes to this bug.