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/pl...
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/pl...
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
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/pl...
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
Hi Levente,
I opened a Mantis issue to make sure this does not get overlooked:
http://bugs.squeak.org/view.php?id=7793
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.
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/pl...
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
On Sat, 19 Oct 2013, David T. Lewis wrote:
Hi Levente,
I opened a Mantis issue to make sure this does not get overlooked:
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/pl...
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
On Sun, Oct 20, 2013 at 05:50:20AM +0200, Levente Uzonyi wrote:
On Sat, 19 Oct 2013, David T. Lewis wrote:
Hi Levente,
I opened a Mantis issue to make sure this does not get overlooked:
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.
Hi Levente,
You mention that the cmake configs for interpreter VM may need to be updated for SqueakSSL, but when I compile the VM on my Linux PC it seems to be fine. I actually do not know how to test the SSL plugin, but it seems to be complete and all of the primitives are present in the module.
lewis@linux-jh8m:~/squeak/build/SqueakSSL> ls -l total 60 drwxr-xr-x 3 lewis users 4096 2013-10-19 12:45 CMakeFiles -rw-r--r-- 1 lewis users 1908 2013-10-19 12:45 cmake_install.cmake -rw-rw-rw- 1 lewis users 509 2013-10-19 12:45 CMakeLists.in -rw-rw-rw- 1 lewis users 683 2013-10-19 12:45 CMakeLists.txt -rw-rw-rw- 1 lewis users 41 2013-10-19 12:45 config.cmake -rw-r--r-- 1 lewis users 11126 2013-10-19 12:45 Makefile -rwxr-xr-x 1 lewis users 27740 2013-10-19 20:35 so.SqueakSSL lewis@linux-jh8m:~/squeak/build/SqueakSSL> size so.SqueakSSL text data bss dec hex filename 17236 1024 48 18308 4784 so.SqueakSSL lewis@linux-jh8m:~/squeak/build/SqueakSSL> nm so.SqueakSSL | grep primitive 0000000000001bc0 T primitiveAccept 0000000000001da0 T primitiveConnect 0000000000001f80 T primitiveCreate 0000000000002000 T primitiveDecrypt 00000000000021e0 T primitiveDestroy 0000000000002280 T primitiveEncrypt 0000000000002460 T primitiveGetIntProperty 0000000000002550 T primitiveGetStringProperty 0000000000002730 T primitiveSetIntProperty 0000000000002840 T primitiveSetStringProperty
This looks like it should work. Is there an easy way that I can test the plugin and verify that it is working?
Thanks, Dave
On Tue, 22 Oct 2013, David T. Lewis wrote:
On Sun, Oct 20, 2013 at 05:50:20AM +0200, Levente Uzonyi wrote:
On Sat, 19 Oct 2013, David T. Lewis wrote:
Hi Levente,
I opened a Mantis issue to make sure this does not get overlooked:
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.
Hi Levente,
You mention that the cmake configs for interpreter VM may need to be updated for SqueakSSL, but when I compile the VM on my Linux PC it seems to be fine. I actually do not know how to test the SSL plugin, but it seems to be complete and all of the primitives are present in the module.
lewis@linux-jh8m:~/squeak/build/SqueakSSL> ls -l total 60 drwxr-xr-x 3 lewis users 4096 2013-10-19 12:45 CMakeFiles -rw-r--r-- 1 lewis users 1908 2013-10-19 12:45 cmake_install.cmake -rw-rw-rw- 1 lewis users 509 2013-10-19 12:45 CMakeLists.in -rw-rw-rw- 1 lewis users 683 2013-10-19 12:45 CMakeLists.txt -rw-rw-rw- 1 lewis users 41 2013-10-19 12:45 config.cmake -rw-r--r-- 1 lewis users 11126 2013-10-19 12:45 Makefile -rwxr-xr-x 1 lewis users 27740 2013-10-19 20:35 so.SqueakSSL lewis@linux-jh8m:~/squeak/build/SqueakSSL> size so.SqueakSSL text data bss dec hex filename 17236 1024 48 18308 4784 so.SqueakSSL lewis@linux-jh8m:~/squeak/build/SqueakSSL> nm so.SqueakSSL | grep primitive 0000000000001bc0 T primitiveAccept 0000000000001da0 T primitiveConnect 0000000000001f80 T primitiveCreate 0000000000002000 T primitiveDecrypt 00000000000021e0 T primitiveDestroy 0000000000002280 T primitiveEncrypt 0000000000002460 T primitiveGetIntProperty 0000000000002550 T primitiveGetStringProperty 0000000000002730 T primitiveSetIntProperty 0000000000002840 T primitiveSetStringProperty
This looks like it should work. Is there an easy way that I can test the plugin and verify that it is working?
The first thing you can do is to check with ldd if so.SqueakSSL actually links to the openssl libraries. If the -lssl and -lcrypto flags are omitted during linking, then the plugin will be compiled, but it will not be functional (all the primitives will be there, but they will fail), because the plugin won't even try to use the openssl libraries.
Other than that, the SqueakSSL-Tests package has some tests (some require internet access to google and yahoo). If you load SqueakSSL-Core and SqueakSSL-Tests, then 12 tests should pass in a Trunk image. Two tests should fail with an error, because Socket >> accept returns a Socket instead of SecureSocket. The solution is to change the method to
Socket >> accept "Accept a connection from the receiver socket. Return a new socket that is connected to the client"
^self class acceptFrom: self
or implement #accept in SecureSocket as
^SecureSocket acceptFrom: self
Levente
Thanks, Dave
On Wed, 23 Oct 2013, Levente Uzonyi wrote:
Other than that, the SqueakSSL-Tests package has some tests (some require internet access to google and yahoo). If you load SqueakSSL-Core and SqueakSSL-Tests, then 12 tests should pass in a Trunk image. Two tests should fail with an error, because Socket >> accept returns a Socket instead of SecureSocket. The solution is to change the method to
Socket >> accept "Accept a connection from the receiver socket. Return a new socket that is connected to the client"
^self class acceptFrom: self
or implement #accept in SecureSocket as
^SecureSocket acceptFrom: self
I have uploaded a new version of SqueakSSL-Core which addresses this, and another issue on Unix: http://leves.web.elte.hu/squeak/SqueakSSL-Core-ul.27.mcz You'll have to delete the file named SqueakSSLCert.pem, if the tests are failing, and you ran the tests before using this version.
In order to run all the SqueakSSL tests, you also have to have WebClient loaded, otherwise some of them will just pass without doing anything.
Levente
Levente
Thanks, Dave
On Wed, Oct 23, 2013 at 04:21:10AM +0200, Levente Uzonyi wrote:
On Tue, 22 Oct 2013, David T. Lewis wrote:
On Sun, Oct 20, 2013 at 05:50:20AM +0200, Levente Uzonyi wrote:
On Sat, 19 Oct 2013, David T. Lewis wrote:
Hi Levente,
I opened a Mantis issue to make sure this does not get overlooked:
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.
Hi Levente,
You mention that the cmake configs for interpreter VM may need to be updated for SqueakSSL, but when I compile the VM on my Linux PC it seems to be fine. I actually do not know how to test the SSL plugin, but it seems to be complete and all of the primitives are present in the module.
lewis@linux-jh8m:~/squeak/build/SqueakSSL> ls -l total 60 drwxr-xr-x 3 lewis users 4096 2013-10-19 12:45 CMakeFiles -rw-r--r-- 1 lewis users 1908 2013-10-19 12:45 cmake_install.cmake -rw-rw-rw- 1 lewis users 509 2013-10-19 12:45 CMakeLists.in -rw-rw-rw- 1 lewis users 683 2013-10-19 12:45 CMakeLists.txt -rw-rw-rw- 1 lewis users 41 2013-10-19 12:45 config.cmake -rw-r--r-- 1 lewis users 11126 2013-10-19 12:45 Makefile -rwxr-xr-x 1 lewis users 27740 2013-10-19 20:35 so.SqueakSSL lewis@linux-jh8m:~/squeak/build/SqueakSSL> size so.SqueakSSL text data bss dec hex filename 17236 1024 48 18308 4784 so.SqueakSSL lewis@linux-jh8m:~/squeak/build/SqueakSSL> nm so.SqueakSSL | grep primitive 0000000000001bc0 T primitiveAccept 0000000000001da0 T primitiveConnect 0000000000001f80 T primitiveCreate 0000000000002000 T primitiveDecrypt 00000000000021e0 T primitiveDestroy 0000000000002280 T primitiveEncrypt 0000000000002460 T primitiveGetIntProperty 0000000000002550 T primitiveGetStringProperty 0000000000002730 T primitiveSetIntProperty 0000000000002840 T primitiveSetStringProperty
This looks like it should work. Is there an easy way that I can test the plugin and verify that it is working?
The first thing you can do is to check with ldd if so.SqueakSSL actually links to the openssl libraries. If the -lssl and -lcrypto flags are omitted during linking, then the plugin will be compiled, but it will not be functional (all the primitives will be there, but they will fail), because the plugin won't even try to use the openssl libraries.
Thanks. I think that Ian has already added the necessary update for cmake, because it is building with the -lssl and -lcrypto flags and ldd confirms that the libraries are being linked.
lewis@linux-jh8m:~/squeak/build/SqueakSSL> ldd so.SqueakSSL linux-vdso.so.1 => (0x00007fff318de000) libssl.so.1.0.0 => /lib64/libssl.so.1.0.0 (0x00007f66ee59c000) libcrypto.so.1.0.0 => /lib64/libcrypto.so.1.0.0 (0x00007f66ee1ec000) libdl.so.2 => /lib64/libdl.so.2 (0x00007f66edfe8000) libz.so.1 => /lib64/libz.so.1 (0x00007f66eddd2000) libc.so.6 => /lib64/libc.so.6 (0x00007f66eda71000) /lib64/ld-linux-x86-64.so.2 (0x00007f66eea25000)
Other than that, the SqueakSSL-Tests package has some tests (some require internet access to google and yahoo). If you load SqueakSSL-Core and SqueakSSL-Tests, then 12 tests should pass in a Trunk image. Two tests should fail with an error, because Socket >> accept returns a Socket instead of SecureSocket. The solution is to change the method to
Socket >> accept "Accept a connection from the receiver socket. Return a new socket that is connected to the client"
^self class acceptFrom: self
or implement #accept in SecureSocket as
^SecureSocket acceptFrom: self
Thanks for the explanations. I am getting errors in the tests, but I think it is due to the plugin not being 64-bit clean (I am compiling as 64-bit). But that is a separate problem, and it looks like the plugin should work as expected on 32-bits. I'll try to compile in 32-bits to confirm when I get some time to install the 32-bit libraries.
Dave
vm-dev@lists.squeakfoundation.org