Last Comment Bug 195914 - NSS startup/shutdown memory leaks..
: NSS startup/shutdown memory leaks..
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.6
: All Windows 2000
: -- normal (vote)
: 3.7.3
Assigned To: Robert Relyea
: Bishakha Banerjee
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-03-04 09:28 PST by Robert Relyea
Modified: 2003-03-18 16:34 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Don't leak the NSS parameter string when adding a new module. (798 bytes, patch)
2003-03-04 09:31 PST, Robert Relyea
wtc: review+
julien.pierre: superreview+
Details | Diff | Splinter Review
Don't leak a session when we login or logout. (519 bytes, patch)
2003-03-04 09:33 PST, Robert Relyea
julien.pierre: superreview-
Details | Diff | Splinter Review
Incorporate Julien's comments (1.08 KB, patch)
2003-03-04 14:51 PST, Robert Relyea
wtc: review-
julien.pierre: superreview+
Details | Diff | Splinter Review
Let's try this patch.... (1.77 KB, patch)
2003-03-04 15:32 PST, Robert Relyea
wtc: review+
julien.pierre: superreview+
Details | Diff | Splinter Review

Description Robert Relyea 2003-03-04 09:28:42 PST
Purify shows several NSS memory leaks in starup and shutdown.
Comment 1 Robert Relyea 2003-03-04 09:29:41 PST
Setting target (actually 3.7.3).
Comment 2 Robert Relyea 2003-03-04 09:31:51 PST
Created attachment 116204 [details] [diff] [review]
Don't leak the NSS parameter string when adding a new module.
Comment 3 Robert Relyea 2003-03-04 09:33:04 PST
Created attachment 116206 [details] [diff] [review]
Don't leak a session when we login or logout.
Comment 4 Julien Pierre 2003-03-04 13:13:55 PST
Comment on attachment 116206 [details] [diff] [review]
Don't leak a session when we login or logout.

1) session should be NULLed after being freed as a safety precaution.
2) Not a comment on the patch, but the block of code just above the patch is
redundant - there are two if tests for (session == NULL) . Only one is needed.
Comment 5 Robert Relyea 2003-03-04 14:51:53 PST
Created attachment 116239 [details] [diff] [review]
Incorporate  Julien's comments

That NULL/NULL is really weird. it's also in C_Login, which is also fixed by
this patch.

NULL'ing out could be a performance issue, but this is login/logout, which
can't be called frequently anyway (where frequently == many times per second).

bob
Comment 6 Wan-Teh Chang 2003-03-04 15:11:47 PST
Comment on attachment 116204 [details] [diff] [review]
Don't leak the NSS parameter string when adding a new module.

r=wtc.	Two questions:
1. In this change:

>     pk11_argSetNewCipherFlags(&ssl[0], ciphers);
>     SECMOD_PUTLONG(encoded->ssl,ssl[0]);
>     SECMOD_PUTLONG(&encoded->ssl[4],ssl[1]);
>+    if (ciphers) PORT_Free(ciphers);

We could free 'ciphers' right after the pk11_argSetNewCipherFlags,
right?

2. I found a problem while reading the code in pk11pars.h.
In pk11_argParseModuleSpec, we should also initialize '*nss'
to 0.
Comment 7 Wan-Teh Chang 2003-03-04 15:19:15 PST
Comment on attachment 116239 [details] [diff] [review]
Incorporate  Julien's comments

The change to NSC_Login is incorrect.  It is freeing
'session' too early, before 'session' is used at line 3343.

The change to NSC_Logout is correct.
Comment 8 Wan-Teh Chang 2003-03-04 15:23:38 PST
Comment on attachment 116239 [details] [diff] [review]
Incorporate  Julien's comments

I just found out that the pk11_FreeSession problem in
NSC_Login is a pre-existing problem, not a change made
by this patch.

It is possible that I misunderstood something.	Maybe
it is safe to free 'session' and continue to use it?
Comment 9 Robert Relyea 2003-03-04 15:24:25 PST
comment #6

Right on both counts.
The later free was just being paranoid, and shouldn't be a problem.
The bug in pk11_argParseModuleSpec should be fixed, but it turns out to be OK
because all of it's callers happen to NULL out the NSS parameter before calling
it anyway. I'll go ahead and check in the *nss = 0 anyway.

bob
Comment 10 Robert Relyea 2003-03-04 15:32:50 PST
Created attachment 116248 [details] [diff] [review]
Let's try this patch....
Comment 11 Wan-Teh Chang 2003-03-04 15:39:46 PST
Comment on attachment 116248 [details] [diff] [review]
Let's try this patch....

r=wtc.	Might be better to declare 'sessionFlags' with
the CK_FLAGS type, but I know CK_FLAGS is the same as
CK_ULONG.
Comment 12 Robert Relyea 2003-03-04 17:24:15 PST
These patches have now been checked into the 3.7 BRANCH and the trunk.

The fix to pk11_argParseModuleSpec is only in the trunk (since it's not a
critical fix).
Comment 13 Wan-Teh Chang 2003-03-18 16:34:41 PST
Set target milestone 3.7.3.

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