Closed
Bug 1235634
Opened 8 years ago
Closed 8 years ago
nsNSSShutDownObjects registered before NSS has been initialized should still be tracked and released upon NSS shutdown
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: keeler, Assigned: ttaubert)
References
(Blocks 1 open bug)
Details
(Whiteboard: [psm-assigned])
Attachments
(1 file, 3 obsolete files)
12.79 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
Currently, if a class that implements nsNSSShutDownObject is instantiated before NSS has been initialized, it won't actually get tracked, so its NSS resources won't be released when NSS shuts down. (For example, this happens with CryptoKey when running the xpcshell test dom/push/test/xpcshell/test_updateRecordNoEncryptionKeys_ws.js. Note that there is a difference between creating an object that implements nsNSSShutDownObject and actually creating the NSS resources it holds. It should be safe to do the former before NSS has been started, as long as the latter never happens before initialization as well.) Luckily, the fix should be straightforward: initialize the nsNSSShutDownObject tracking list upon first use.
Assignee | ||
Comment 1•8 years ago
|
||
Reporter | ||
Comment 2•8 years ago
|
||
Comment on attachment 8731679 [details] [diff] [review] 0001-Bug-1235634-Construct-nsNSSShutdownList-singleton-la.patch Review of attachment 8731679 [details] [diff] [review]: ----------------------------------------------------------------- This is the right idea, but I'm concerned about some thread safety issues (see comment below). ::: security/manager/ssl/nsNSSShutDown.cpp @@ +140,5 @@ > } > > nsNSSShutDownList *nsNSSShutDownList::construct() > { > + if (!singleton) { I think singleton needs to be protected by a StaticMutex (we should be able to convert nsNSSShutDownList::mListLock for this purpose, I imagine). Also, nsNSSComponent has a handle on the shut down list in mShutdownObjectList - this should be protected by the mutex as well (actually, even better, have nsNSSComponent only interact with the shut down list via static methods).
Attachment #8731679 -
Flags: review?(dkeeler) → review-
Assignee | ||
Comment 3•8 years ago
|
||
I think/hope this should address all thread-safety issues.
Attachment #8731679 -
Attachment is obsolete: true
Attachment #8732111 -
Flags: review?(dkeeler)
Reporter | ||
Comment 4•8 years ago
|
||
Comment on attachment 8732111 [details] [diff] [review] 0001-Bug-1235634-Construct-nsNSSShutdownList-singleton-la.patch, v2 Review of attachment 8732111 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. I noticed another preexisting potential race condition, but I think the solution is pretty simple. I think I should have another look at the final patch just to be sure, so r- for now. ::: security/manager/ssl/nsNSSShutDown.cpp @@ +94,5 @@ > + if (!singleton) { > + return NS_ERROR_FAILURE; > + } > + > + MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("canceling all open SSL sockets to disallow future IO\n")); nit: let's also break up this line so it's <80 characters while we're here ::: security/manager/ssl/nsNSSShutDown.h @@ +61,5 @@ > // which hold NSS resources and support the "early cleanup mechanism". > class nsNSSShutDownList > { > public: > ~nsNSSShutDownList(); Looks like this destructor doesn't need to be public anymore. @@ +84,4 @@ > > static nsNSSActivityState *getActivityState() > { > return singleton ? &singleton->mActivityState : nullptr; I think we still have an issue here (or rather, this has always been an issue, but luckily it hasn't bit us yet). It looks like the only thing that uses this is nsNSSShutDownPreventionLock (in the constructor/destructor). Let's remove this and add two static methods here (enterActivityState and leaveActivityState) that nsNSSShutDownPreventionLock can call. Those two methods can then hold the list lock while calling mActivityState.enter/leave. @@ +89,5 @@ > > private: > nsNSSShutDownList(); > > + static nsNSSShutDownList *construct(); construct doesn't need to return anything now, right? Also, since we only call it where we already need to acquire the list lock, let's have it take a reference to an unnamed StaticMutexAutoLock so we don't have to acquire it twice.
Attachment #8732111 -
Flags: review?(dkeeler) → review-
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #4) > ::: security/manager/ssl/nsNSSShutDown.h > @@ +61,5 @@ > > // which hold NSS resources and support the "early cleanup mechanism". > > class nsNSSShutDownList > > { > > public: > > ~nsNSSShutDownList(); > > Looks like this destructor doesn't need to be public anymore. Indeed, moved. > @@ +84,4 @@ > > > > static nsNSSActivityState *getActivityState() > > { > > return singleton ? &singleton->mActivityState : nullptr; > > I think we still have an issue here (or rather, this has always been an > issue, but luckily it hasn't bit us yet). It looks like the only thing that > uses this is nsNSSShutDownPreventionLock (in the constructor/destructor). > Let's remove this and add two static methods here (enterActivityState and > leaveActivityState) that nsNSSShutDownPreventionLock can call. Those two > methods can then hold the list lock while calling mActivityState.enter/leave. Good spot, fixed. > @@ +89,5 @@ > > > > private: > > nsNSSShutDownList(); > > > > + static nsNSSShutDownList *construct(); > > construct doesn't need to return anything now, right? > Also, since we only call it where we already need to acquire the list lock, > let's have it take a reference to an unnamed StaticMutexAutoLock so we don't > have to acquire it twice. Great idea, done.
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8732111 -
Attachment is obsolete: true
Attachment #8732787 -
Flags: review?(dkeeler)
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8732787 [details] [diff] [review] 0001-Bug-1235634-Construct-nsNSSShutdownList-singleton-la.patch, v3 Review of attachment 8732787 [details] [diff] [review]: ----------------------------------------------------------------- Great - r=me with comments addressed. ::: security/manager/ssl/nsNSSShutDown.cpp @@ +91,5 @@ > nsresult nsNSSShutDownList::doPK11Logout() > { > + StaticMutexAutoLock lock(sListLock); > + if (!singleton) { > + return NS_ERROR_FAILURE; I think this actually wouldn't be a failure - it just means we never created any applicable resources tracked by this system. @@ +120,5 @@ > nsresult nsNSSShutDownList::evaporateAllNSSResources() > { > + StaticMutexAutoLock lock(sListLock); > + if (!singleton) { > + return NS_ERROR_FAILURE; Same here. @@ +158,5 @@ > > +void nsNSSShutDownList::enterActivityState() > +{ > + StaticMutexAutoLock lock(sListLock); > + if (singleton) { We should probably call construct to make sure it actually exists here.
Attachment #8732787 -
Flags: review?(dkeeler) → review+
Comment 9•8 years ago
|
||
backed out for testfailures/leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=24334731&repo=mozilla-inbound
Flags: needinfo?(ttaubert)
Comment 10•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/7c465dd93903
Assignee | ||
Comment 11•8 years ago
|
||
nsNSSShutDownList::enterActivityState() is called after ::shutdown(), so we have to account for that.
Flags: needinfo?(ttaubert)
Assignee | ||
Comment 12•8 years ago
|
||
Not calling ::construct() in ::enterActivityState() doesn't seem to prevent the leak, although this is the only method I can see being called after ::shutdown(). Is it possible that the mutex here is destroyed too late for the leak checker? It probably doesn't know about the nsNSSShutDownList object as that's not refcounted.
Reporter | ||
Updated•8 years ago
|
Whiteboard: [psm-assigned]
Assignee | ||
Comment 13•8 years ago
|
||
It seems that the leaked Mutex and CondVar are coming from mActivityState. If I remove that we don't leak any longer. That's a little confusing as nsNSSShutdownList::shutdown() is being called at the same time we previously called its destructor.
Assignee | ||
Comment 14•8 years ago
|
||
Ok, that took me a long time to figure out. When moving to a lazily created nsNSSShutdownList we create an instance in the child too, which we didn't do before. We however don't clean up in the child and so we leak that instance.
Assignee | ||
Comment 15•8 years ago
|
||
So how could we ensure cleanup in child processes? It looks like there's no nsNSSComponent instance, otherwise ::shutdown() would've been called, right? David, any idea?
Flags: needinfo?(dkeeler)
Reporter | ||
Comment 16•8 years ago
|
||
Since NSS is running in "no DB" mode in the child, I don't think we actually need to clean up and shut down NSS resources there. This is nice because it simplifies some things, but on the other hand there's no way to tell if we're leaking NSS resources in that case. Anyway, I think we can just make all of the nsNSSShutDownWhatever operations a no-op if we're not in the parent process.
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 17•8 years ago
|
||
Ok, here's a new version that doesn't leak anymore. It achieves that by not creating an instance after ::shutdown() was called or when we're in a child process.
Attachment #8732787 -
Attachment is obsolete: true
Attachment #8741072 -
Flags: review?(dkeeler)
Reporter | ||
Comment 18•8 years ago
|
||
Comment on attachment 8741072 [details] [diff] [review] 0001-Bug-1235634-Construct-nsNSSShutdownList-singleton-la.patch, v4 Review of attachment 8741072 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me (of course, I've said that before, but hopefully this time I'm right). ::: security/manager/ssl/nsNSSShutDown.cpp @@ +53,5 @@ > > void nsNSSShutDownList::remember(nsNSSShutDownObject *o) > { > + StaticMutexAutoLock lock(sListLock); > + if (!nsNSSShutDownList::construct(lock)) nit: let's add the braces around the conditional body
Attachment #8741072 -
Flags: review?(dkeeler) → review+
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/77d53bf103a2
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
•