Fix various memory leaks in allocating strings. #918
Conversation
The fixes are limited to what are covered by tests/TestRedis.php
Command used to detect memory leaks (Anything mentioning redis):
```bash
USE_ZEND_ALLOC=0 ZEND_DONT_UNLOAD_MODULES=1 valgrind --leak-check=full \
php --php-ini php.ini tests/TestRedis.php
```
- USE_ZEND_ALLOC uses malloc/free/etc instead of Zend's custom allocator
- ZEND_DONT_UNLOAD_MODULES allows valgrind to print the file and line
numbers for a shared library (this extension)
- about valgrind: http://valgrind.org/docs/manual/quick-start.html
While the allocated memory would be cleaned up after the end of a request,
memory leaks might cause problems for long-running CLI scripts dealing
with hundreds of thousands of keys.
|
This was an awesome pull request, thank you! You're totally right about using Cheers! |
| } else { | ||
| add_assoc_string(&z_sub_result, key, value); | ||
| } | ||
| efree(value); // Either way, it doesn't use the original string. |
There was a problem hiding this comment.
Does add_assoc_string duplicate value?
There was a problem hiding this comment.
It does in the php7 framework:
https://php-lxr.adamharvey.name/source/xref/PHP-7.0/Zend/zend_API.c#1396
There was a problem hiding this comment.
If you want to avoid duplicating strings unnecessarily, use the zend_strings instead of char* : See zend_hash_add: https://php-lxr.adamharvey.name/source/xref/PHP-7.0/Zend/zend_hash.h#80
(May be missing a layer of pointers or pointer dereferencing in this example)
// if this is the only zval referring to that array
HashTable*ht = ZVAL_ARR(z_subresult); // outside the inner loop over properties,
zend_hash_add(ht, key, value);(This may involve incrementing ref counts if used multiple times, or it might be done automatically if used as array keys, I'm not sure)
The fixes are limited to what are covered by tests/TestRedis.php
Command used to detect memory leaks (Anything mentioning redis):
USE_ZEND_ALLOC=0 ZEND_DONT_UNLOAD_MODULES=1 valgrind --leak-check=full \ php --php-ini php.ini tests/TestRedis.phpnumbers for a shared library (this extension)
While the allocated memory would be cleaned up after the end of a request,
memory leaks might cause problems for long-running CLI scripts dealing
with hundreds of thousands of keys.
zval_get_string will do one of the following for a ZVAL:
In both cases, zend_string_release should be called on the result.
Several other places not listed in this PR should also be using zval_get_string instead of convert_to_string.