391770 - OCSP_Global.monitor is leaked on shutdown

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This bug is filed based on the NSS_3_12_ALPHA1B tag, which is what is currently used by the trunk of Firefox, etc.

OCSP_Global.monitor is not freed on shutdown.  The following patch fixes the leak, although I don't know if this is a sensible place to free it or not.  (It seems a little odd to enter/exit it and then free it; if it's correct it perhaps implies that the enter/exit are unneeded.)

--- a/security/nss/lib/certhigh/ocsp.c
+++ b/security/nss/lib/certhigh/ocsp.c
@@ -889,6 +889,10 @@ SECStatus OCSP_ShutdownCache(void)
     OCSP_Global.cache.MRUitem = NULL;
     OCSP_Global.cache.LRUitem = NULL;
     PR_ExitMonitor(OCSP_Global.monitor);
+
+    PR_DestroyMonitor(OCSP_Global.monitor);
+    OCSP_Global.monitor = NULL;
+
     return SECSuccess;
 }

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

The fix for this bug should not be committed while it remains blocked by 
bug 391815, and bug 391815 remains unresolved.

Comment 2

[Slavomir Katuscak]

This leak is already detected by our tools, dependency on bug 391815 can be removed.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: DBaron's patch

Kai, please review. 
Please comment on David's question concerning if this is the right
place to destroy the monitor, or not.

Comment 4

[Kai Engert [:KaiE:]]

I agree to this patch.

But the monitor protects additional information.
As a consequence, while NSS is in the "shutdown state" and the monitor not available, some functions calls will fail with an error message.

I think this is ok, because NSS does not support any activity while it's in the "shutdown state".

As we're now shutting down more than just the OCSP cache, we should rename the function and call it OCSP_ShutdownGlobal (matching the existing OCSP_InitGlobal).

I wonder if the additional preferences (including an application registered callback pointer) should be kept across NSS sessions, or whether we should clean them during shutdown.

Comment 5

[Kai Engert [:KaiE:]]

(In reply to comment #0)
> It
> seems a little odd to enter/exit it and then free it; if it's correct it
> perhaps implies that the enter/exit are unneeded.

With the existing code that leaks the small amount to hold the global monitor, this problem would never show up, and I personally think, such a global one-time leak should be ok to use. The same monitor would be re-used across multiple NSS sessions (as in profile switching).

But if you prefer to fix this leak, we must discuss what's likely and what not.

Will PSM's XPCOM code successfully be able to prevent any calls into NSS while NSS is in the shutdown state? I'm not sure. It seems difficult to guarantee it.

An application *should* only call into nss-shutdown after it stopped using NSS, but maybe the application will do it anyway.

I think it doesn't hurt to hold the monitor while we are freeing the data. This will make it less likely that multiple threads access the same data.

By holding the monitor we ensure we will not crash during cleanup, but would crash when another thread attempts to enter/exit the monitor. This might be a more obvious hint to the real problem. (A crash during cleanup could look like a heap corruption)

Comment 6

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Taking bug.
I propose this improved patch.

Comment 7

[Kai Engert [:KaiE:]]

Attachment: Patch v3

Same patch, but this one applies cleanly to the trunk (context changed)

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Kai, please also attach a patch to remove the associated leak stack from
the file security/nss/tests/memleak/ignored .  
I will review it at the same time.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to list if ignored leak stacks

Kai, I think this is the set of leak stacks that your patch will fix.
Do you agree?

Comment 10

[Kai Engert [:KaiE:]]

Comment on attachment 283294 [details] [diff] [review]
patch to list if ignored leak stacks

(In reply to comment #9)
> Kai, I think this is the set of leak stacks that your patch will fix.
> Do you agree?

yes

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 283223 [details] [diff] [review]
Patch v3

OK, then, Kai, please commit both your patch v3 and the patch to the  ignored leaks file at the same time.  Thanks.

Going forward, all patches that eliminate leaks should also include the modification to the "ignored" file to remove (or change) the affected leak stacks.

Comment 12

[Kai Engert [:KaiE:]]

Checked in, marking fixed. Thanks David!

/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.43; previous revision: 1.42

/cvsroot/mozilla/security/nss/lib/certhigh/ocspi.h,v  <--  ocspi.h
new revision: 1.7; previous revision: 1.6

/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.83; previous revision: 1.82

/cvsroot/mozilla/security/nss/tests/memleak/ignored,v  <--  ignored
new revision: 1.25; previous revision: 1.24