nrk / phpiredis

PHP extension for Redis based on Hiredis
BSD 2-Clause "Simplified" License
490 stars 67 forks source link

Memory leak #10

Closed romainneutron closed 11 years ago

romainneutron commented 11 years ago

Hello,

When running the following script with PHP compiled with the debug option (for example by installing it with brew install php54 --enable-debug), I got a memory leak detection by zend engine :

$reader = phpiredis_reader_create();
phpiredis_reader_set_status_handler($reader, function(){});
phpiredis_reader_set_error_handler($reader, function(){});
phpiredis_reader_destroy($reader);

outputs :

[Fri Dec 28 13:05:29 2012]  Script:  '/Users/romain/Documents/workspace/GloubsterServer/testleak.php'
Zend/zend_vm_execute.h(6804) :  Freeing 0x1042F46A0 (32 bytes), script=/Users/romain/Documents/workspace/GloubsterServer/testleak.php
Last leak repeated 1 time
[Fri Dec 28 13:05:29 2012]  Script:  '/Users/romain/Documents/workspace/GloubsterServer/testleak.php'
/private/tmp/php54-WbUP/php-5.4.10/Zend/zend_API.c(3078) :  Freeing 0x1042F71C0 (18 bytes), script=/Users/romain/Documents/workspace/GloubsterServer/testleak.php
Last leak repeated 1 time
[Fri Dec 28 13:05:29 2012]  Script:  '/Users/romain/Documents/workspace/GloubsterServer/testleak.php'
/usr/local/src/phpiredis/phpiredis.c(194) :  Freeing 0x1042F7D30 (40 bytes), script=/Users/romain/Documents/workspace/GloubsterServer/testleak.php
=== Total 5 memory leaks detected ===
romainneutron commented 11 years ago

FYI, to know if PHP is compiled with the debug flag, just run php -v, the debug flag is noted as follows :

PHP 5.4.10 (cli) (built: Dec 26 2012 12:18:20) (DEBUG)
Copyright (c) 1997-2012 The PHP Group
Zend Engine v2.4.0, Copyright (c) 1998-2012 Zend Technologies
    with Xdebug v2.2.1, Copyright (c) 2002-2012, by Derick Rethans
nrk commented 11 years ago

Just to keep track of my guess explained on IRC, I think the bug lies in phpiredis_reader_destroy() in which we should free up the memory allocated for the two reader callbacks but instead we only destroy the underlying hiredis reader resource.

I'll try fixing this one ASAP, thanks!

nrk commented 11 years ago

@romainneutron alright it wasn't exactly what I expected but I fixed those leaks anyway, I pushed a fix in the fix-memoryleak-gh10 branch. I tested it with PHP 5.3 and PHP 5.4 and I don't get warnings anymore, can you double check that everything works for you?

PS: btw the repository has been transferred so if you don't want to clone it again from scratch you should point to the new URL using git remote set-url.

romainneutron commented 11 years ago

I confirm that the issue is solved with your branch :rainbow:

nrk commented 11 years ago

Awesome, merged into master. Thanks!