Hi!
There is a new finding to this. There is one more place for porbable memory
leak. The earlier solution which I proposed(using a funtion
SSL_free_comp_methods) is usfficient to arrest the memory leaks if zlib is
not being dynamically loaded.
if I enable -DZLIB -DZLIB_SHARED in openssl build, and run the simple
application mentioned in earlier post through valgrind, This is the output
==23867== 86 bytes in 5 blocks are still reachable in loss record 5 of 6
==23867== at 0x1B903CC8: malloc (vg_replace_malloc.c:131)
==23867== by 0x1B983840: default_malloc_ex (mem.c:79)
==23867== by 0x1B983EF6: CRYPTO_malloc (mem.c:304)
==23867== by 0x1B9E24A9: DSO_new_method (dso_lib.c:103)
|
|
|
==23867== LEAK SUMMARY:
==23867== definitely lost: 0 bytes in 0 blocks.
==23867== possibly lost: 0 bytes in 0 blocks.
==23867== still reachable: 804 bytes in 10 blocks.
==23867== suppressed: 0 bytes in 0 blocks.
On further investigation, I found that the variable "zlib_dso" which get
allocated during call to COMP_zlib(), is not being free through any control
path.
I wrote a funtion like this in the file c_zlib.c and called it before
calling SSL_free_comp_methods() as part of the cleanup sequence from
application.
/* defined in c_zlib.c *.
387 void UNLOAD_zlib(void)
388 {
389 if(zlib_dso != NULL)
390 DSO_free(zlib_dso);
391 }
With this change, I do not get the "still reachable" error message from the
valgrind run.
regards
-Nitin
From: Darryl Miles <[EMAIL PROTECTED]>
Reply-To: openssl-dev@openssl.org
To: openssl-dev@openssl.org
Subject: Re: Memory Leaks in SSL_Library_init()
Date: Mon, 02 Apr 2007 18:56:40 +0100
Nitin M wrote:
Darryl, What is your opinion on this finding? As you were also keen on
knowing the result of this experimentation. :)
>>
What is you opinion?
Here is the valgrind output for the above program for your reference.
==10877== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 17 from 1)
--10877--
--10877-- supp: 17 Ugly strchr error in /lib/ld-2.3.2.so
==10877== malloc/free: in use at exit: 36 bytes in 2 blocks.
==10877== malloc/free: 89 allocs, 87 frees, 1632 bytes allocated.
==10877==
==10877== searching for pointers to 2 not-freed blocks.
==10877== checked 2852348 bytes.
Here is my output from 0.9.7f:
--5448-- supp: 4 dl_relocate_object
==5448== malloc/free: in use at exit: 0 bytes in 0 blocks.
==5448== malloc/free: 79 allocs, 79 frees, 2,640 bytes allocated.
No leaks, but you are testing with 0.9.8, which version?
I don't have time at the minute to investigate with 0.9.8.
I'd also like to see a SSL_library_cleanup(), which in turn calls
SSL_free_comp_methods() just to start the ball rolling on reducing the
OpenSSL voodoo.
This is still true. The cleanup is an area of the library which could do
with the rough edges taking off it and the officially agreed methods to be
documented in the same place as SSL_library_init().
>> With a call "SSL_library_cleanup()", which internally calls
>> EVP_cleanup() and SSL_free_comp_methods() do you see any harm or any
>> scenario in which it might break. I feel that if used properly for
>> every corresponding SSL_library_init(), it should not causse any
problem.
Agreed on this. A function "SSL_library_cleanup()" should be created,
which will reverse the action of calling SSL_library_init() and using the
OpenSSL library in any way possible.
It is fine by me to document that the SSL_library_cleanup() is not
thread-safe to clearly warn users to ensure they revert back to single
threaded access to the OpenSSL library for calling cleanup functions. It
should also be documented that the application code should have already
free'ed any OpenSSL objects it created and retained before calling
SSL_library_cleanup(). If should be documented that the state of the
OpenSSL library reverts to the same state as when an process first loads
the library, so the same rules apply again (i.e. its maybe necessary to
call SSL_library_init() before making use of some OpenSSL provided
functions).
It is fine by me to implement the necessary locking within the
sub-functions it calls to make those respective function thread-safe (since
there maybe legitimate reasons to call them independently of
SSL_library_cleanup() function) I think this is already done anyway.
It is fine by me for the proposed SSL_free_comp_methods() to be implemented
and also fine to remove the previously discussed first to lines from the
posted patch which attempt to reduce lock contention and locking overhead.
You will have to petition one of the project comitters to actually get any
action on these points. Maybe posting a fresh patch which adds the above
and the necessary new documentation will help get things moving.
<rant_mode>I already have an outstanding contribution for the project that
still remains unaddressed (see postings on SSL_shutdown()) so although this
issue is something I'd normally pickup on and try to formalize (as I think
I understand the issue well enough to do that) I can only contribute as
fast as the consortium allows. My extra energy which would otherwise be
spent assisting the greater good is eaten up by maintaining a local
fork.</rant_mode>
Darryl
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-dev@openssl.org
Automated List Manager [EMAIL PROTECTED]
_________________________________________________________________
Catch the complete World Cup coverage with MSN
http://content.msn.co.in/Sports/Cricket/Default.aspx
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-dev@openssl.org
Automated List Manager [EMAIL PROTECTED]
