Closed
Bug 1251801
Opened 8 years ago
Closed 8 years ago
Address misc issues regarding nsPK11TokenDB and nsPK11Token
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
(3 files)
nsPK11TokenDB and nsPK11Token have various issues between them: - Don't implement nsNSSShutDownObject at all or don't fully implement it by not checking that NSS isn't already shutdown in some methods - Don't use smart pointers for all NSS resource management - Misc issues like unnecessary variables and use of goto These should be fixed.
Assignee | ||
Updated•8 years ago
|
Summary: Fully implement nsNSSShutDownObject and obviate manual NSS resource management in nsPK11TokenDB and nsPK11Token → Addess misc issues regarding nsPK11TokenDB and nsPK11Token
Assignee | ||
Updated•8 years ago
|
Summary: Addess misc issues regarding nsPK11TokenDB and nsPK11Token → Address misc issues regarding nsPK11TokenDB and nsPK11Token
Whiteboard: [psm-assigned]
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42597/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42597/
Attachment #8735104 -
Flags: review?(dkeeler)
Attachment #8735105 -
Flags: review?(dkeeler)
Attachment #8735106 -
Flags: review?(dkeeler)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42599/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42599/
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42601/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42601/
Assignee | ||
Comment 4•8 years ago
|
||
https://reviewboard.mozilla.org/r/42599/#review39105 ::: security/manager/ssl/nsPK11TokenDB.cpp:256 (Diff revision 1) > - // PK11_MapError sets CKR_USER_NOT_LOGGED_IN to SEC_ERROR_LIBRARY_FAILURE, > + return MapSECStatus(PK11_Logout(mSlot.get())); > - // so not going to learn anything here by a failure. Treat it like void. > - PK11_Logout(mSlot.get()); > - return NS_OK; Two things to note here: 1. PK11_MapError() now maps to the more reasonable SEC_ERROR_TOKEN_NOT_LOGGED_IN. 2. I'm not sure if the point of this comment is: 2a. To say that PK11_Logout() can "fail" if the user hasn't logged in, and that we should ignore any errors regardless? 2b. That SEC_ERROR_LIBRARY_FAILURE is a bad error to return, but that it's ok to return something a more informative error?
Attachment #8735104 -
Flags: review?(dkeeler) → review+
Comment on attachment 8735104 [details] MozReview Request: Bug 1251801 - Fully implement nsNSSShutDownObject and obviate manual NSS resource management. r=keeler https://reviewboard.mozilla.org/r/42597/#review39647 Looks great - sorry it took a while for me to get to this.
Comment on attachment 8735105 [details] MozReview Request: Bug 1251801 - Improve handling of PK11_* function error codes. r=keeler https://reviewboard.mozilla.org/r/42599/#review39649
Attachment #8735105 -
Flags: review?(dkeeler) → review+
Comment on attachment 8735106 [details] MozReview Request: Bug 1251801 - Ensure arguments of all public methods are checked. r=keeler https://reviewboard.mozilla.org/r/42601/#review39653
Attachment #8735106 -
Flags: review?(dkeeler) → review+
(In reply to :Cykesiopka from comment #4) > https://reviewboard.mozilla.org/r/42599/#review39105 > > ::: security/manager/ssl/nsPK11TokenDB.cpp:256 > (Diff revision 1) > > - // PK11_MapError sets CKR_USER_NOT_LOGGED_IN to SEC_ERROR_LIBRARY_FAILURE, > > + return MapSECStatus(PK11_Logout(mSlot.get())); > > - // so not going to learn anything here by a failure. Treat it like void. > > - PK11_Logout(mSlot.get()); > > - return NS_OK; > > Two things to note here: > 1. PK11_MapError() now maps to the more reasonable > SEC_ERROR_TOKEN_NOT_LOGGED_IN. > 2. I'm not sure if the point of this comment is: > 2a. To say that PK11_Logout() can "fail" if the user hasn't logged in, > and that we should ignore any errors regardless? Oh, actually, now that I take another look at this, it seems like the code that calls this isn't expecting it to throw in the case that the user isn't logged in. We should probably keep the original behavior.
Assignee | ||
Comment 9•8 years ago
|
||
https://reviewboard.mozilla.org/r/42597/#review39647 No worries. Thanks for the review!
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #8) > (In reply to :Cykesiopka from comment #4) > > https://reviewboard.mozilla.org/r/42599/#review39105 > > > > ::: security/manager/ssl/nsPK11TokenDB.cpp:256 > > (Diff revision 1) > > > - // PK11_MapError sets CKR_USER_NOT_LOGGED_IN to SEC_ERROR_LIBRARY_FAILURE, > > > + return MapSECStatus(PK11_Logout(mSlot.get())); > > > - // so not going to learn anything here by a failure. Treat it like void. > > > - PK11_Logout(mSlot.get()); > > > - return NS_OK; > > > > Two things to note here: > > 1. PK11_MapError() now maps to the more reasonable > > SEC_ERROR_TOKEN_NOT_LOGGED_IN. > > 2. I'm not sure if the point of this comment is: > > 2a. To say that PK11_Logout() can "fail" if the user hasn't logged in, > > and that we should ignore any errors regardless? > > Oh, actually, now that I take another look at this, it seems like the code > that calls this isn't expecting it to throw in the case that the user isn't > logged in. We should probably keep the original behavior. Sure. I'll preserve the original behaviour (but update the comment etc to be clearer about why). I'll skip adding a test for this case for now, but I'll add one in my changes for Bug 1252722.
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8735104 [details] MozReview Request: Bug 1251801 - Fully implement nsNSSShutDownObject and obviate manual NSS resource management. r=keeler Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42597/diff/1-2/
Attachment #8735104 -
Attachment description: MozReview Request: Bug 1251801 - Fully implement nsNSSShutDownObject and obviate manual NSS resource management. → MozReview Request: Bug 1251801 - Fully implement nsNSSShutDownObject and obviate manual NSS resource management. r=keeler
Attachment #8735105 -
Attachment description: MozReview Request: Bug 1251801 - Improve handling of PK11_* function error codes. → MozReview Request: Bug 1251801 - Improve handling of PK11_* function error codes. r=keeler
Attachment #8735106 -
Attachment description: MozReview Request: Bug 1251801 - Ensure arguments of all public methods are checked. → MozReview Request: Bug 1251801 - Ensure arguments of all public methods are checked. r=keeler
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8735105 [details] MozReview Request: Bug 1251801 - Improve handling of PK11_* function error codes. r=keeler Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42599/diff/1-2/
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8735106 [details] MozReview Request: Bug 1251801 - Ensure arguments of all public methods are checked. r=keeler Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42601/diff/1-2/
Assignee | ||
Comment 14•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d09cea9d6a3b34d4a5171ffb07c79e19784bfa1
Keywords: checkin-needed
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ccacd83b165 https://hg.mozilla.org/integration/mozilla-inbound/rev/667fa766321f https://hg.mozilla.org/integration/mozilla-inbound/rev/8dea713c9fcd
Keywords: checkin-needed
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5ccacd83b165 https://hg.mozilla.org/mozilla-central/rev/667fa766321f https://hg.mozilla.org/mozilla-central/rev/8dea713c9fcd
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
•