Closed Bug 195914 Opened 22 years ago Closed 22 years ago

NSS startup/shutdown memory leaks..

Categories

(NSS :: Libraries, defect)

All
Windows 2000
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rrelyea, Assigned: rrelyea)

Details

Attachments

(2 files, 2 obsolete files)

Purify shows several NSS memory leaks in starup and shutdown.
Setting target (actually 3.7.3).
Target Milestone: --- → 3.7.2
Attachment #116204 - Flags: superreview?(jpierre)
Attachment #116204 - Flags: review?(wtc)
Attachment #116206 - Flags: superreview?(jpierre)
Attachment #116206 - Flags: review?(wtc)
Attachment #116204 - Flags: superreview?(jpierre) → superreview+
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-
Attached patch Incorporate Julien's comments (obsolete) — Splinter Review
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
Attachment #116239 - Flags: superreview?(jpierre)
Attachment #116239 - Flags: review?(wtc)
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 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 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 #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
Attachment #116239 - Attachment is obsolete: true
Attachment #116248 - Flags: superreview?(jpierre)
Attachment #116248 - Flags: review?(wtc)
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+
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
Closed: 22 years ago
Resolution: --- → FIXED
Attachment #116239 - Flags: superreview?(jpierre) → superreview+
Attachment #116248 - Flags: superreview?(jpierre) → superreview+
Whiteboard: [3.7.3]
Target Milestone: 3.7.2 → ---
Set target milestone 3.7.3.
Whiteboard: [3.7.3]
Target Milestone: --- → 3.7.3
Attachment #116206 - Flags: review?(wchang0222)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: