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; }
Priority: -- → P2
Target Milestone: --- → 3.12
Version: 3.12 → trunk
The fix for this bug should not be committed while it remains blocked by bug 391815, and bug 391815 remains unresolved.
This leak is already detected by our tools, dependency on bug 391815 can be removed.
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.
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.
(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)
Created attachment 283220 [details] [diff] [review] Patch v2 Taking bug. I propose this improved patch.
Created attachment 283223 [details] [diff] [review] Patch v3 Same patch, but this one applies cleanly to the trunk (context changed)
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.
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?
Attachment #283294 - Flags: review?(kengert)
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
Attachment #283294 - Flags: review?(kengert) → review+
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.
Attachment #283223 - Flags: review?(nelson) → review+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.