1233853 - nsISyncJPAKE implementation (nsSyncJPAKE) needs to be aware of NSS shutdown

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

nsSyncJPAKE uses NSS resources but isn't aware that NSS may shut down, making those resources unsafe to use (as a result it also leaks these resources when NSS is shut down).

Another improvement would be for it to use RAII types instead of manually releasing resources.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: MozReview Request: bug 1233853 - make nsSyncJPAKE aware of NSS shutdown r?jcj

Review commit: https://reviewboard.mozilla.org/r/28611/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28611/

Comment 2

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8700195 [details]
MozReview Request: bug 1233853 - make nsSyncJPAKE aware of NSS shutdown r?jcj

https://reviewboard.mozilla.org/r/28611/#review25935

It seems to me that all is well, except maybe the null check I have down on nsSyncJPAKE.cpp:462. Please check me :)

Thanks!

::: services/crypto/component/nsSyncJPAKE.cpp:462
(Diff revision 1)
> +  key.dispose();

It looks like, if `Round1` hasn't run by the time this is called, that `key` has a chance to be `nullptr`.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://reviewboard.mozilla.org/r/28611/#review25935

> It looks like, if `Round1` hasn't run by the time this is called, that `key` has a chance to be `nullptr`.

ScopedPK11SymKey is defined using MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE, which should mean that dispose calls release, which does a null check: https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/mfbt/Scoped.h#284 . Since that's a mess of templates and macros, I could be reading that wrong, though, so if you could double-check, that would be great.

Comment 4

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8700195 [details]
MozReview Request: bug 1233853 - make nsSyncJPAKE aware of NSS shutdown r?jcj

https://reviewboard.mozilla.org/r/28611/#review25979

LGTM.

Comment 5

[J.C. Jones [:jcj] (he/they)]

https://reviewboard.mozilla.org/r/28611/#review25935

> ScopedPK11SymKey is defined using MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE, which should mean that dispose calls release, which does a null check: https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/mfbt/Scoped.h#284 . Since that's a mess of templates and macros, I could be reading that wrong, though, so if you could double-check, that would be great.

Blasted C++ assignment operator, gets me every time. Yes, `key` the object can't be null.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63bb81bf8517

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks!

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63bb81bf8517

Comment 9

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS				Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected