195914 - NSS startup/shutdown memory leaks..

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Robert Relyea]

Purify shows several NSS memory leaks in starup and shutdown.

Comment 1

[Robert Relyea]

Setting target (actually 3.7.3).

Comment 2

[Robert Relyea]

Attachment: Don't leak the NSS parameter string when adding a new module.

Comment 3

[Robert Relyea]

Attachment: Don't leak a session when we login or logout.

Comment 4

[Julien Pierre]

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]

Attachment: 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]

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]

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]

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]

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]

Attachment: Let's try this patch....

Comment 11

[Wan-Teh Chang]

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]

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]

Set target milestone 3.7.3.