NSS startup/shutdown memory leaks..

RESOLVED FIXED in 3.7.3

Status

NSS
Libraries
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Robert Relyea, Assigned: Robert Relyea)

Tracking

3.7.3
All
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

14 years ago
Purify shows several NSS memory leaks in starup and shutdown.
(Assignee)

Comment 1

14 years ago
Setting target (actually 3.7.3).
Target Milestone: --- → 3.7.2
(Assignee)

Comment 2

14 years ago
Created attachment 116204 [details] [diff] [review]
Don't leak the NSS parameter string when adding a new module.
(Assignee)

Comment 3

14 years ago
Created attachment 116206 [details] [diff] [review]
Don't leak a session when we login or logout.
(Assignee)

Updated

14 years ago
Attachment #116204 - Flags: superreview?(jpierre)
Attachment #116204 - Flags: review?(wtc)
(Assignee)

Updated

14 years ago
Attachment #116206 - Flags: superreview?(jpierre)
Attachment #116206 - Flags: review?(wtc)

Updated

14 years ago
Attachment #116204 - Flags: superreview?(jpierre) → superreview+

Comment 4

14 years ago
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.
Attachment #116206 - Flags: superreview?(jpierre) → superreview-
(Assignee)

Comment 5

14 years ago
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
Attachment #116206 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #116239 - Flags: superreview?(jpierre)
Attachment #116239 - Flags: review?(wtc)

Comment 6

14 years ago
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.
Attachment #116204 - Flags: review?(wtc) → review+

Comment 7

14 years ago
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.
Attachment #116239 - Flags: review?(wtc) → review-

Comment 8

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

Comment 9

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

Comment 10

14 years ago
Created attachment 116248 [details] [diff] [review]
Let's try this patch....
Attachment #116239 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #116248 - Flags: superreview?(jpierre)
Attachment #116248 - Flags: review?(wtc)

Comment 11

14 years ago
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.
Attachment #116248 - Flags: review?(wtc) → review+
(Assignee)

Comment 12

14 years ago
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).
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Attachment #116239 - Flags: superreview?(jpierre) → superreview+

Updated

14 years ago
Attachment #116248 - Flags: superreview?(jpierre) → superreview+

Updated

14 years ago
Whiteboard: [3.7.3]
Target Milestone: 3.7.2 → ---

Comment 13

14 years ago
Set target milestone 3.7.3.
Whiteboard: [3.7.3]
Target Milestone: --- → 3.7.3
(Assignee)

Updated

14 years ago
Attachment #116206 - Flags: review?(wchang0222)
You need to log in before you can comment on or make changes to this bug.