Last Comment Bug 391770 - OCSP_Global.monitor is leaked on shutdown
: OCSP_Global.monitor is leaked on shutdown
Status: RESOLVED FIXED
: mlk
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: x86 Windows XP
: P2 trivial (vote)
: 3.12
Assigned To: Kai Engert (:kaie)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-10 16:36 PDT by David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
Modified: 2007-10-04 06:05 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
DBaron's patch (606 bytes, patch)
2007-10-01 12:01 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Splinter Review
Patch v2 (3.55 KB, patch)
2007-10-02 11:42 PDT, Kai Engert (:kaie)
no flags Details | Diff | Splinter Review
Patch v3 (3.56 KB, patch)
2007-10-02 11:47 PDT, Kai Engert (:kaie)
nelson: review+
Details | Diff | Splinter Review
patch to list if ignored leak stacks (1.07 KB, patch)
2007-10-02 19:53 PDT, Nelson Bolyard (seldom reads bugmail)
kaie: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2007-08-10 16:36:08 PDT
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) 2007-08-11 01:56:17 PDT
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 2007-10-01 06:20:39 PDT
This leak is already detected by our tools, dependency on bug 391815 can be removed.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2007-10-01 12:01:38 PDT
Created attachment 283041 [details] [diff] [review]
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) 2007-10-02 11:21:15 PDT
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) 2007-10-02 11:36:58 PDT
(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) 2007-10-02 11:42:06 PDT
Created attachment 283220 [details] [diff] [review]
Patch v2

Taking bug.
I propose this improved patch.
Comment 7 Kai Engert (:kaie) 2007-10-02 11:47:54 PDT
Created attachment 283223 [details] [diff] [review]
Patch v3

Same patch, but this one applies cleanly to the trunk (context changed)
Comment 8 Nelson Bolyard (seldom reads bugmail) 2007-10-02 17:53:03 PDT
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) 2007-10-02 19:53:20 PDT
Created attachment 283294 [details] [diff] [review]
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) 2007-10-03 05:11:03 PDT
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) 2007-10-03 14:19:01 PDT
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) 2007-10-04 06:05:48 PDT
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

Note You need to log in before you can comment on or make changes to this bug.