Closed Bug 1251801 Opened 4 years ago Closed 4 years ago

Address misc issues regarding nsPK11TokenDB and nsPK11Token

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

(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.
Summary: Fully implement nsNSSShutDownObject and obviate manual NSS resource management in nsPK11TokenDB and nsPK11Token → Addess misc issues regarding nsPK11TokenDB and nsPK11Token
Depends on: 1259149
Summary: Addess misc issues regarding nsPK11TokenDB and nsPK11Token → Address misc issues regarding nsPK11TokenDB and nsPK11Token
Whiteboard: [psm-assigned]
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?
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.
https://reviewboard.mozilla.org/r/42597/#review39647

No worries. Thanks for the review!
(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.
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
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/
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/
You need to log in before you can comment on or make changes to this bug.