Closed
Bug 1252722
Opened 8 years ago
Closed 8 years ago
Address misc issues regarding nsPKCS11Slot and nsPKCS11ModuleDB
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: Cykesiopka, Assigned: Cykesiopka)
References
Details
(Whiteboard: [psm-assigned])
Attachments
(5 files)
58 bytes,
text/x-review-board-request
|
keeler
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
keeler
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
keeler
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
keeler
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
keeler
:
review+
|
Details |
Basically, fix all the things mentioned in Bug 1251801 comment 0, but for nsPKCS11Slot and nsPKCS11ModuleDB.
Assignee | ||
Updated•8 years ago
|
Summary: Fully implement nsNSSShutDownObject and obviate manual NSS resource management in nsPKCS11Slot and nsPKCS11ModuleDB → Addess misc issues regarding nsPKCS11Slot and nsPKCS11ModuleDB
Assignee | ||
Updated•8 years ago
|
Summary: Addess misc issues regarding nsPKCS11Slot and nsPKCS11ModuleDB → Address misc issues regarding nsPKCS11Slot and nsPKCS11ModuleDB
Whiteboard: [psm-assigned]
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43409/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43409/
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43411/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43411/
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43413/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43413/
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43415/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43415/
Attachment #8736577 -
Flags: review?(dkeeler) → review+
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+
Assignee | ||
Comment 11•8 years ago
|
||
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.
Assignee | ||
Comment 12•8 years ago
|
||
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.
Assignee | ||
Comment 13•8 years ago
|
||
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
Assignee | ||
Comment 14•8 years ago
|
||
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/
Assignee | ||
Comment 15•8 years ago
|
||
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/
Assignee | ||
Comment 16•8 years ago
|
||
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/
Assignee | ||
Comment 17•8 years ago
|
||
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/
Assignee | ||
Comment 18•8 years ago
|
||
Thanks for the review! https://treeherder.mozilla.org/#/jobs?repo=try&revision=b469ed0a803d2bd0cfc5c559b1c7577214f9428f
Keywords: checkin-needed
Comment 19•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/419899bf45c5 https://hg.mozilla.org/integration/mozilla-inbound/rev/64c333e9b12a https://hg.mozilla.org/integration/mozilla-inbound/rev/0f8ee183f2cd https://hg.mozilla.org/integration/mozilla-inbound/rev/71b02a7c2340 https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7aec22dfef
Keywords: checkin-needed
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/419899bf45c5 https://hg.mozilla.org/mozilla-central/rev/64c333e9b12a https://hg.mozilla.org/mozilla-central/rev/0f8ee183f2cd https://hg.mozilla.org/mozilla-central/rev/71b02a7c2340 https://hg.mozilla.org/mozilla-central/rev/0e7aec22dfef
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•