418365 - Memory leaks in selfserv related to TCP sockets.

Status: RESOLVED FIXED

(NSS :: Test, defect, P3)

Description

[Slavomir Katuscak]

Build:
Tinderbox - securitytip memleak SunOS/sparc ciotat on 2008/02/18 21:46:00
DBs upgraded to Shared DB

---
Checking for memory leaks...

Actual leaks report    (actual leaks:            2  total size:         48 bytes)

Memory Leak (mel):
Found leaked block of size 24 bytes at address 0x465c8
At time of allocation, the call stack was:
    [1] _PR_Getfd() at 0xe6d083fc
    [2] PR_NewTCPSocket() at 0xe6d2eb88
    [3] main() at 0x192b0

Memory Leak (mel):
Found leaked block of size 24 bytes at address 0x46598
At time of allocation, the call stack was:
    [1] _PR_Getfd() at 0xe6d083e8
    [2] PR_NewTCPSocket() at 0xe6d2eb88
    [3] main() at 0x192b0
---

There are only 2 calls of PR_NewTCPSocket in selfserv code - for listen_sock and  model_sock, seems that they were not closed on exit.

Comment 1

[Slavomir Katuscak]

Adding pattern to ignore list:
selfserv/main/PR_NewTCPSocket/**

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Slavo, please back out that change to the "ignored" file.
We don't want to ignore leaks that happen rarely.  
We only want to ignore leaks that happen repeatedly.  

Comment 3

[Slavomir Katuscak]

Commented out. I would rather keep it there to have complete list of found stacks and to prevent that someone will add it there again. 

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

OK, thanks.

Comment 5

[Julien Pierre]

Slavo,

Did this leak happen on an optimized build only ? Some frames are obviously missing.

Was there any error from selfserv in the log when this leak was reported ?

I am going to guess that the one socket that was leaked was the listen socket created in getBoundListenSocket. It's possible that this function failed setting one of the options (unlikely), binding (more likely, see bug 348198) or listening. We leak the socket in these error paths. But I would think we would also leak a few more things, since we don't cleanup NSPR. That's not consistent with this leak report. So, I looked a little bit further in the code.

It's possible that we leaked the socket if NSS initialization failed, policy setup, cipher string error, missing server cert or key, bypass setup failure, or NSS shutdown failure, or logger thread creation failure. Did any of these occur ?

I will attach a patch to fix the leak within getBoundListenSocket, but for the other exit paths, this would require significantly changing the structure of selfserv, and there are other leaks on exit. So I will leave the other cases alone for now.

Comment 6

[Julien Pierre]

Attachment: Don't leak listen socket in error cases in getBoundListenSocket()

Comment 7

[Slavomir Katuscak]

(In reply to comment #5)
> Slavo,
> 
> Did this leak happen on an optimized build only ? Some frames are obviously
> missing.

Happened only once in optimized build.

> Was there any error from selfserv in the log when this leak was reported ?

Good question, but I don't have answer for it. Selfserv was running over DBX and DBX log is already lost, but it's possible that it was some special case with some failure.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 305908 [details] [diff] [review]
Don't leak listen socket in error cases in getBoundListenSocket()

This is fine.

In general, I am not concerned about leaks that occur in the NSS test programs
(rather than in the libraries) when they occur only in cases of test failure.
If the test has failed, then the tree is going to go orange because it failed,
regardless of whether some leak is present or not.  A leak that occurs in a 
test program that has succeeded is worthy of a bug report and an "ignored" 
stack.  Such a leak makes Tinderbox go orange even though all the tests pass.
Leaks that occur only when tests fail should not be put into the 
ignored file, and I wouldn't file any bugs on them, either.

If this bug is reporting a leak that occurs only when the test fails for some
other reason, then the priority of this bug can go down to P4, or it can even
be resolved as WONTFIX.

Comment 9

[Julien Pierre]

Nelson,

Thanks for the review. There is no sure way to tell if this was an error case here since the log is gone.

I checked the patch in to the trunk :

Checking in selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.82; previous revision: 1.81
done

Slavo, if this error can be reproduced, please reopen this bug, and attach relevant error log information.

Comment 10

[Slavomir Katuscak]

As this bug is already fixed, I'm removing it from list of ignored leaks.