The php7 api changed from previous versions in that there is no
longer the option to "duplicate or not duplicate" a string value
when using the ZVAL_STRING and ZVAL_STRINGL macros.
This means that every time ZVAL_STRING or ZVAL_STRINGL is being
called, one *must* call zval_dtor on that zval or there will be
a leak.
In addition there were several places where the return value from
a zend_call_function passthrough was not being freed, and then
being used again. Specifically (in the context of RedisArray)
it was when the array was already in MULTI mode, meaning that the
return is a refcounted copy of the Redis object itself, which also
needs to be freed to avoid a leak.
All unit tests passing in debug/release mode with no memory leaks
being reported or any complaining from valgrind.
The php7 api has substantial changes in the way one goes about
interacting with the zval type (php's internal structure for dealing
with dynamically typed variables).
Most notably, most everything is dereferenced by one pointer. So,
where you once used zval** you typically use zval*, and where you
used zval* you now just use a zval struct on the stack.
In addition, the api changed in how you return a string. Previously
you had the option of whether or not to "duplicate" the memory returned
(or in other words, pass ownership of the pointer to PHP or not).
Because phpredis sometimes buffers commands to the server (say in the
case of a pipeline, or a MULTI/EXEC) transaction, we had several stack
violations and/or memory corruption which resulted from keeping track
of the address of a stack allocated structure, which was accessed at
a later date (sometimes causing segmentation faults, bus errors, etc).
Also, there were a few places where this pattern was being used:
zval **ptr = emalloc(sizeof(zval*) * count);
/* stuff */
ZVAL_STRING(ptr[0], "hello world");
Given that ZVAL_STRING() thinks it's writing to an allocated structure
it would cause various random issues (because we were blowing through
the data segment and writing into the code section).
This commit (so far) fixes all of the segmentation faults and memory
errors but there are still leaks (specifically in RedisArray) that need
to be solved.
Addresses #727
* When converting zval from heap -> stack allocation some frees were left, causing invalid frees on stack values
* zend_parse* was use int instead of size_t when consuming strings, causing undefined behavior
* Properly handle single array as well as variadic arguments for
things like RedisCluster::del
* Wrapping keys in {} such that Redis and RedisCluster tests can
use the same methods (avoiding CROSSSLOT).
* Fixed a double-free scenerio in redis_array_impl.c
The pattern to move a key for various types (strings, sets, zsets,
hashes, etc) used a simple pattern:
1. Construct the call in order to get all of the keys from the source
2. Make a pass through call to the source node to get a response
3. Use the response to make a pass through call to the destination node
The issue, however, was that we were using the same return value variable
for both source and destination nodes, so we would leak the response from
the source node.
Conflicts:
redis_array_impl.c
The pattern to move a key for various types (strings, sets, zsets,
hashes, etc) used a simple pattern:
1. Construct the call in order to get all of the keys from the source
2. Make a pass through call to the source node to get a response
3. Use the response to make a pass through call to the destination node
The issue, however, was that we were using the same return value variable
for both source and destination nodes, so we would leak the response from
the source node.
RedisArray will segfault if you pass something other than strings
into your array of hosts. This is invalid input (needs strings here)
but it shouldn't crash php.
This brings the W32 compilation fixes done by @char101 up to date and
allows building of php_redis.dll with VC11 on Win32 (allows for a php5.5
version).
I accidentally pulled this when getting some of the pull requests
integrated (git flow style) for this release. I like the idea
for sure, but I think it needs more detailed documentation and
further testing.
At the very least, I need to understand it :)
