129218 - memory leaks in tstclnt and strsclnt

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Bishakha Banerjee]

The call stack is as follows:

Memory leak
12 bytes allocated by calloc in prmem.c (64), HANDLE: 0x004B2E08

Location of Error
PR_Calloc
prmem.c
64
PL_CreateOptState
plgetopt.c
75
main
tstclnt.c
317
_mainCRTStartup
TSTCLNT.EXE!00007EFD
(unknown)

Comment 1

[Wan-Teh Chang]

Assigned the bug to Bishakha.  Changed the component to Tools.
Target 3.4.1, priority P2.

Comment 2

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 3

[Wan-Teh Chang]

Set target milestone to NSS 3.5.

Comment 4

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

The SSL QA test clint program tstclnt has a number of leaks.
These leaks are not cause by the NSS shared libraries, but are in the
test program itself.  The program should be cleaned up to eliminate
these leaks.  At one time, bugs were filed for each individual leak.
I'm going to dup all but one of those bugs to this one.  

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 129219 has been marked as a duplicate of this bug. ***

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 129220 has been marked as a duplicate of this bug. ***

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 129223 has been marked as a duplicate of this bug. ***

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 129224 has been marked as a duplicate of this bug. ***

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 129225 has been marked as a duplicate of this bug. ***

Comment 12

[Slavomir Katuscak]

Solaris DBX output:

Actual leaks report    (actual leaks:            4  total size:         61 bytes)

Memory Leak (mel):
Found leaked block of size 24 bytes at address 0x48fa8
At time of allocation, the call stack was:
	[1] PR_Malloc() at line 467 in "prmem.c"
	[2] PL_CreateOptState() at line 83 in "plgetopt.c"
	[3] main() at line 1349 in "strsclnt.c"

Memory Leak (mel):
Found leaked block of size 21 bytes at address 0x48fd8
At time of allocation, the call stack was:
	[1] PL_strdup() at line 53 in "strdup.c"
	[2] main() at line 1403 in "strsclnt.c"

Memory Leak (mel):
Found leaked block of size 12 bytes at address 0x46c20
At time of allocation, the call stack was:
	[1] calloc() at 0xe0937488 
	[2] PR_Calloc() at line 475 in "prmem.c"
	[3] PL_CreateOptState() at line 78 in "plgetopt.c"
	[4] main() at line 1349 in "strsclnt.c"

Memory Leak (mel):
Found leaked block of size 4 bytes at address 0x47598
At time of allocation, the call stack was:
	[1] PL_strdup() at line 53 in "strdup.c"
	[2] main() at line 1397 in "strsclnt.c"

Comment 13

[Alexei Volkov]

Attachment: patch for leak in strsclnt.c/main/PL_CreateOptState

call PL_DestroyOptState after done parsing argv

Comment 14

[Wan-Teh Chang]

Comment on attachment 257395 [details] [diff] [review]
patch for leak in strsclnt.c/main/PL_CreateOptState

r=wtc.

Comment 15

[Alexei Volkov]

Integrating attachment 257395 [details] [diff] [review]:
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.52; previous revision: 1.51

Comment 16

[Alexei Volkov]

Attachment: fix leaks created by strsclnt.c/main/PL_strdup

free strings allocated by PL_strdup

Comment 17

[Neil Williams]

Comment on attachment 257404 [details] [diff] [review]
fix leaks created by strsclnt.c/main/PL_strdup

One patch for all the leaks would be nice.

Comment 18

[Alexei Volkov]

Sorry, first one is already integrated into the trunk.

Second patch is integrated:
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.53; previous revision: 1.52

Comment 19

[Wan-Teh Chang]

Alexei, this bug was originally reported against tstclnt.c,
but your two patches modify strsclnt.c.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

reopening for two reasons:
a) the bug was reported against tstclnt, but the patches fix strsclnt
b) since we're doing our leak tests against the 3.11 branch, the fixes 
need to go on that branch.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Giving this bug to Alexei, since he wrote the patches.

Comment 22

[Slavomir Katuscak]

This problem seems to be in all 3 tools used for ciphers testing - selfserv, strsclnt, tstclnt. Tstclnt is normally not tested for memory leaks (it doesn't test ciphers, just send commands to selfserv). 

For selfserv there was opened bug 366557. 

I put leaks from memleak testing there in comment 12, because stack looks the same (PL_CreateOptState called from main function and not cleaned) and I haven't realized that it's different tool. Strsclnt seems to be fixed now, so there is not needed to create new bug. Next thing is to fix tstclnt.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #22)
> Tstclnt is normally not tested for memory leaks (it doesn't
> test ciphers, just send commands to selfserv). 

Tstclnt certainly does test ciphers.  
We use it to test every one of the cipher suites.

> Strsclnt seems to be fixed now, so
> there is not needed to create new bug. Next thing is to fix tstclnt.

Agreed.

Comment 24

[Slavomir Katuscak]

(In reply to comment #23)
> 
> Tstclnt certainly does test ciphers.  
> We use it to test every one of the cipher suites.

Current memleak tests uses tstclnt only for selfserv shutdown. (Sends command "GET /stop HTTP/1.0".) 

Do we need to add there something more ?

Comment 25

[Alexei Volkov]

Attachment: patch for tstclnt.c/main/strdup leaks

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 257562 [details] [diff] [review]
patch for tstclnt.c/main/strdup leaks

r=nelson
I don't know that this patch plugs all the leaks, but it looks like it plugs several.

Comment 27

[Alexei Volkov]

committing attachment 257562 [details] [diff] [review] into trunk:

/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.48; previous revision: 1.47

I think this patch fixes all know mem leaks in tstclnt/strsclnt. Slavo, please reopen the bug if here are more.

Comment 28

[Alexei Volkov]

Attachment: fixing presumably last leak in strsclnt

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 258319 [details] [diff] [review]
fixing presumably last leak in strsclnt

I wouldn't bet that this is the last leak (:-), but this is a good fix. 
r=nelson

Comment 30

[Neil Williams]

Comment on attachment 258319 [details] [diff] [review]
fixing presumably last leak in strsclnt

Alexei, you confused me making patches for both strsclnt and tstclnt. You may say I confuse easily but the title does say "leak in tstclnt".

Comment 31

[Alexei Volkov]

integrating attachment 258319 [details] [diff] [review] into trunk

/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.55; previous revision: 1.54

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

committed on 3.11 branch,
Checking in strsclnt.c; new revision: 1.43.2.8; previous revision: 1.43.2.7