Closed Bug 1235634 Opened 4 years ago Closed 4 years ago

nsNSSShutDownObjects registered before NSS has been initialized should still be tracked and released upon NSS shutdown

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- affected
firefox48 --- fixed

People

(Reporter: keeler, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file, 3 obsolete files)

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: dkeeler → ttaubert
Status: NEW → ASSIGNED
Attachment #8731679 - Flags: review?(dkeeler)
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-
I think/hope this should address all thread-safety issues.
Attachment #8731679 - Attachment is obsolete: true
Attachment #8732111 - Flags: review?(dkeeler)
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-
(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.
Attachment #8732111 - Attachment is obsolete: true
Attachment #8732787 - Flags: review?(dkeeler)
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+
backed out for testfailures/leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=24334731&repo=mozilla-inbound
Flags: needinfo?(ttaubert)
nsNSSShutDownList::enterActivityState() is called after ::shutdown(), so we have to account for that.
Flags: needinfo?(ttaubert)
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.
Whiteboard: [psm-assigned]
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.
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.
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)
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)
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)
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+
https://hg.mozilla.org/mozilla-central/rev/77d53bf103a2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.