The default bug view has changed. See this FAQ.

OCSP_Global.monitor is leaked on shutdown

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P2
trivial
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: dbaron, Assigned: kaie)

Tracking

({mlk})

trunk
3.12
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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
Depends on: 391815
The fix for this bug should not be committed while it remains blocked by 
bug 391815, and bug 391815 remains unresolved.
Assignee: nobody → nelson

Comment 2

10 years ago
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.
Attachment #283041 - Flags: review?(kengert)
No longer depends on: 391815
(Assignee)

Comment 4

10 years ago
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.
(Assignee)

Comment 5

10 years ago
(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)
(Assignee)

Comment 6

10 years ago
Created attachment 283220 [details] [diff] [review]
Patch v2

Taking bug.
I propose this improved patch.
Assignee: nelson → kengert
Attachment #283041 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #283220 - Flags: review?(nelson)
Attachment #283041 - Flags: review?(kengert)
(Assignee)

Comment 7

10 years ago
Created attachment 283223 [details] [diff] [review]
Patch v3

Same patch, but this one applies cleanly to the trunk (context changed)
Attachment #283220 - Attachment is obsolete: true
Attachment #283223 - Flags: review?(nelson)
Attachment #283220 - Flags: review?(nelson)
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)
(Assignee)

Comment 10

10 years ago
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+
(Assignee)

Comment 12

10 years ago
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: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.