[Vm-dev] Memory leak in the SqueakSSL plugin on unix

[Levente Uzonyi]

On Sat, 19 Oct 2013, David T. Lewis wrote:

>
> Hi Levente,
>
> I opened a Mantis issue to make sure this does not get overlooked:
>
>  http://bugs.squeak.org/view.php?id=7793

Thanks. I've attached the file with the changes.

>
> Do you now have an updated sqUnixOpenSSL.c that fixes the memory leak
> problem? If so can you please post the file here (or attach it to the Mantis
> report)? I understand the changes from your original message but I'm not
> clear if more work needs to be done related to "the BIO_free_all calls are
> not needed in general".
>
> The sqUnixOpenSSL.c file is identical on trunk and oscog, so we can apply
> your fix to both branches.

I made some changes, and it seems like the leakage is gone. I only ran a 
short test of a few thousand connections, but I didn't experience any
increase in memory usage with the new code. You can find the modified file 
here: http://leves.web.elte.hu/squeak/sqUnixOpenSSL.c

Both the cmake (for the interpreter) and the automake (for Cog) configs 
will have to be created/updated to be able to build a functional plugin.


Levente

>
> Thanks!
> Dave
>
>
> On Thu, Oct 17, 2013 at 11:50:32PM +0200, Levente Uzonyi wrote:
>>
>> I tried to set up a build environment to be able to build a functional
>> SqueakSSL plugin and test this idea. The only barrier was that the plugin
>> is not linked to libssl nor to libcrypto, so it won't do anything. You can
>> use ldd to check if this is the case with your plugin too:
>>
>> $ ldd SqueakSSL
>> 	linux-gate.so.1 =>  (0xf7763000)
>> 	libc.so.6 => /lib/i386-linux-gnu/libc.so.6 (0xf7595000)
>> 	/lib/ld-linux.so.2 (0xf7764000)
>>
>> This is currently the case with Eliot's build.
>> To make the plugin work I added -lssl and -lcrypto manually to the
>> Makefile. I have no idea how to add it to autoconf, so someone else will
>> have to do that. With those flags ldd will show something like this:
>>
>> $ ldd SqueakSSL
>> 	linux-gate.so.1 =>  (0xf77c0000)
>> 	libssl.so.1.0.0 => /lib/i386-linux-gnu/libssl.so.1.0.0 (0xf7760000)
>> 	libcrypto.so.1.0.0 => /lib/i386-linux-gnu/libcrypto.so.1.0.0
>> 	(0xf75b5000)
>> 	libc.so.6 => /lib/i386-linux-gnu/libc.so.6 (0xf740b000)
>> 	libdl.so.2 => /lib/i386-linux-gnu/libdl.so.2 (0xf7406000)
>> 	libz.so.1 => /lib/i386-linux-gnu/libz.so.1 (0xf73f0000)
>> 	/lib/ld-linux.so.2 (0xf77c1000)
>>
>> It quickly turned out that the BIO_free_all calls are not needed in
>> general, only when the ssl structure doesn't get allocated, because
>> SSL_free frees both bioRead and bioWrite.
>>
>>
>> Levente
>>
>> On Thu, 17 Oct 2013, Levente Uzonyi wrote:
>>
>>>
>>> Seems like the end of the message got lost. So here's the full message
>>> again:
>>>
>>> Hello,
>>>
>>> we've been experiencing memory leakage in long running Squeak images using
>>> SqueakSSL. After a bit of monitoring I found that 132 bytes get leaked
>>> for each https request done from the image. After a bit of code review,
>>> I've probably found the culprit, and another potential source of memory
>>> leak. For the reference, the source file this mail is about is
>>> http://squeakvm.org/cgi-bin/viewvc.cgi/squeak/branches/Cog/platforms/unix/plugins/SqueakSSL/sqUnixOpenSSL.c?revision=2713&view=markup
>>>
>>> The main memory leak is in sqDestroySSL function (starting on line 117),
>>> which doesn't free the bioRead and bioWrite variables (allocated by
>>> sqCreateSSL on line 98-99) of the ssl object. My suggested solution is to
>>> insert the following two lines before line 132:
>>>
>>>       BIO_free_all(ssl->bioRead);
>>>       BIO_free_all(ssl->bioWrite);
>>>
>>> The other potential source of memory leak is sqSetStringPropertySSL
>>> (starting on line 381). It allocates a chunk of memory on line 389, but
>>> doesn't use nor free it, if the propID argument is not
>>> SQSSL_PROP_CERTNAME. My suggested solution is to insert the following line
>>> after line 396:
>>>
>>>       if(property) free(property);
>>>
>>> Note that I haven't tested any of these, but I hope someone who is more
>>> into VM building right now will try them.
>>>
>>> Cheers,
>>> Levente
>>>
>