Segfault in RedisArray test #727

szepeviktor · 2016-01-13T18:15:30Z

# tests/mkring.sh start
# php tests/TestRedis.php --class RedisArray
Note: these tests might take up to a minute. Don't worry :-)
Testing class RedisArray

WITH per-node index:
testMSet           Segmentation fault

How to report this error?
Debian jessie, PHP 7.0

The text was updated successfully, but these errors were encountered:

szepeviktor · 2016-01-13T18:32:13Z

This is another one

# tests/make-cluster.sh start
# php tests/TestRedis.php --class RedisCluster
Note: these tests might take up to a minute. Don't worry :-)
Testing class RedisCluster
testSortAsc                [SKIPPED]
testSortDesc               [SKIPPED]
testWait                   [SKIPPED]
testSelect                 [SKIPPED]
testReconnectSelect        [SKIPPED]
testPing                   Segmentation fault

Debian jessie, PHP 7.0

michael-grunder · 2016-01-13T18:35:48Z

I can replicate the RedisArray segfault and there is something weird going on with the php7 tests. I must have broken something in the past few days.

Will confirm when a fix is up.

Jerry-Shaw · 2016-01-15T10:03:20Z

I got my cli stopped working and crashed in this test...

michael-grunder · 2016-01-15T17:21:04Z

Quick update for you guys. There are actually a few things that are rather broken in the php7 branch. I've fixed several but there are a few more.

If anyone cares, it has to do with the "great dereferencing" in the php7 api. Essentially, the api has changed in php7 such that any zval ** -> zval *, and zval * -> zval. Specifically, what's going on is that there are several places where the address of stack allocated zval structures are being saved, and then attempted to be freed at a later point (which is bad 😺)

I should have these fixed over the weekend

Cheers
Mike

szepeviktor · 2016-01-15T18:50:34Z

I'm just planning to switch to 7.0,
so you have time to fix these.
Thanks.

The php7 api has substantial cha 8000 nges 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

michael-grunder · 2016-01-25T21:04:23Z

Hey, @szepeviktor and @Jerry-Shaw. My tests are all passing now using the above commit, and I don't see any complaining from valgrind (but I'm still testing). However, there are still quite a few memory leaks with the RedisArray class which I will solve with another commit.

I just wanted to get this up so you could test it as well.

Cheers!
Mike

szepeviktor · 2016-01-25T21:06:06Z

Thank you!

Jerry-Shaw · 2016-01-26T06:02:57Z

Good, thanks. I'll test it when I have time, little busy these days.

szepeviktor · 2016-01-26T07:25:00Z

On a fresh Debian 8.2:

/root/phpredis-php7/cluster_library.c: In function 'cluster_multibulk_resp_recursive':
/root/phpredis-php7/cluster_library.c:139:60: warning: passing argument 4 of 'redis_sock_gets' from incompatible pointer type
                 if(redis_sock_gets(sock, buf, sizeof(buf), &r->len) < 0) {
                                                            ^
In file included from /root/phpredis-php7/cluster_library.c:3:0:
/root/phpredis-php7/library.h:18:19: note: expected 'size_t *' but argument is of type 'long long int *'
 PHP_REDIS_API int redis_sock_gets(RedisSock *redis_sock, char *buf, int buf_size, size_t* line_len TSRMLS_DC);
                   ^

All tests passed. \o/ 😄

And after fixing nodemap location cluster test says:

$ php tests/TestRedis.php --class RedisCluster
Note: these tests might take up to a minute. Don't worry :-)
Testing class RedisCluster
testSortAsc                [SKIPPED]
testSortDesc               [SKIPPED]
testWait                   [SKIPPED]
testSelect                 [SKIPPED]
testReconnectSelect        [SKIPPED]
testPing                   [PASSED]
testRandomKey              [FAILED]
testEcho                   [PASSED]
testSortPrefix             Segmentation fault

szepeviktor · 2016-01-26T07:29:14Z

~~Could it be that I run cluster tests from CLI, not from a browser?~~ No we've talked about that $_SERVER has real values in CLI also.
https://github.com/phpredis/phpredis/blob/php7/tests/RedisClusterTest.php#L38

Solution: dirname(dirname($_SERVER['PHP_SELF'])) . '/nodes/nodemap'

michael-grunder · 2016-01-26T19:05:13Z

Ok strange. I don't get any segmentation fault and this is breaking right where it did for me before I updated the code. Are you sure you've recompiled with the updates?

It's just odd that it's breaking in the exact same spot.

michael-grunder · 2016-01-26T19:08:02Z

In fact, when I run it from the previous commit I see what you're seeing:

➜  redis git:(24ce1c3) ✗ php7 tests/TestRedis.php --class RedisCluster
Note: these tests might take up to a minute. Don't worry :-)
Testing class RedisCluster
testSortAsc                [SKIPPED]
testSortDesc               [SKIPPED]
testWait                   [SKIPPED]
testSelect                 [SKIPPED]
testReconnectSelect        [SKIPPED]
testPing                   [PASSED]
testRandomKey              [PASSED]
testEcho                   [PASSED]
testSortPrefix             zend_mm_heap corrupted
[1]    60353 segmentation fault  php7 tests/TestRedis.php --class RedisCluster

With the new code no seg fault, and just one failed test.

szepeviktor · 2016-01-26T19:29:03Z

The last test was on a fresh Debian, now it is a fresh install also.

`cd /root/phpredis/; php tests/TestRedis.php --class RedisCluster`

michael-grunder · 2016-01-26T19:51:59Z

Hey thanks for that. I am seeing the same thing myself on that system.

How are you building/installing this version of phpredis. Are you using the standard:

phpize
./configure
make && sudo make install

I don't want to mess around on your box :)

szepeviktor · 2016-01-26T20:22:56Z

Yes, exactly the dame way ss in the readme.
Log in and type history and you'll see all typed commands.

You may do anything on this box, it was spun up only for phpredis.

+36204242498
Ezen a készüléken nehéz gépelni.
Elnézést!

On January 26, 2016 8:52:22 PM CET, Michael Grunder notifications@github.com wrote:

Hey thanks for that. I am seeing the same thing myself on that system.

How are you building this version of php. Are you using the standard:
phpize
./configure
make && sudo make install
I don't want to mess around on your box :)

Reply to this email directly or view it on GitHub:
#727 (comment)

michael-grunder · 2016-01-26T20:34:11Z

OK I will see what I can find. This could be a debug vs production build issue. I got a core dump and am taking a peek now.

More info soon.

Thanks for the help! :)

szepeviktor · 2016-01-27T11:26:46Z

That virtual server is deleted.

szepeviktor · 2016-01-27T11:29:07Z

Try using Travis with Ubuntu 14, it is free and easy

This is what I've done:

apt-get install build-essential make git redis-server ruby-redis ruby-dev php7.0-dev php7.0-cli
git clone https://github.com/phpredis/phpredis.git && cd phpredis/
git checkout php7
phpize
./configure
make && make install
cp -v /usr/share/doc/redis-tools/examples/redis-trib.rb /usr/local/bin/
# Add dirname() in tests/RedisClusterTest.php
echo -e "; priority=20\nextension=redis.so" > /etc/php/mods-available/redis.ini
phpenmod -v 7.0 -s ALL redis && php -m|grep redis
# Run tests!

https://github.com/phpredis/phpredis/blob/php7/README.markdown#running-the-unit-tests

starikovs · 2016-02-02T08:31:17Z

@szepeviktor, I have the same warning while compiling the extension. I use the php7 branch.

/usr/src/php/ext/redis/cluster_library.c: In function 'cluster_multibulk_resp_recursive':

/usr/src/php/ext/redis/cluster_library.c:139:60: warning: passing argument 4 of 'redis_sock_gets' from incompatible pointer type

                 if(redis_sock_gets(sock, buf, sizeof(buf), &r->len) < 0) {

                                                            ^

In file included from /usr/src/php/ext/redis/cluster_library.c:3:0:

/usr/src/php/ext/redis/library.h:18:19: note: expected 'size_t *' but argument is of type 'long long int *'

 PHP_REDIS_API int redis_sock_gets(RedisSock *redis_sock, char *buf, int buf_size, size_t* line_len TSRMLS_DC);

I just worry if it's safe to use after these errors.

szepeviktor · 2016-02-02T12:44:34Z

@starikovs I think these warnings are innocent but should be fixed.

Jerry-Shaw · 2016-02-03T04:55:08Z

Ah.. Hi, I still get redisArray Test failed and cli crashed under Win x64.
Something error on the connections? The host forced to shut down the connection is shown.
Only testCreateSecondRing passed.

There were a few more places mixing emalloc(sizeof(zval*)*N) with emalloc(sizeof(zval)*N causing us to overwrite data in the stack. These didn't appear until the code was built in release mode. Now to build in debug mode to get rid of any leaks. Addresses #727

michael-grunder · 2016-02-05T18:13:27Z

Ok guys, please try again. More fun times overwriting the stack frame which causes the best kind of errors to debug 😺 These particular errors didn't show up (at least for me) until I built in release mode.

There are still some outstanding memory leaks I need to sort so be aware but now all tests pass for me for Redis, RedisArray, and RedisCluster.

I will test more running the code through valgrind and then switch back to a debug build so I can track down the leaks.

Cheers!
Mike

szepeviktor · 2016-02-05T18:27:14Z

# php tests/TestRedis.php --class RedisCluster
Note: these tests might take up to a minute. Don't worry :-)
Testing class RedisCluster
testSortAsc                [SKIPPED]
testSortDesc               [SKIPPED]
testWait                   [SKIPPED]
testSelect                 [SKIPPED]
testReconnectSelect        [SKIPPED]
testPing                   [PASSED]
testRandomKey              [FAILED]
testEcho                   [PASSED]
testSortPrefix             [PASSED]
testDBSize                 [PASSED]
testInfo                   [PASSED]
testClient                 [PASSED]
testTime                   [PASSED]
testScan                   [PASSED]
testPubSub                 [PASSED]
testMSetNX                 [PASSED]
testSlowlog                [PASSED]
testInfoCommandStats       [PASSED]
testFailedTransactions     [PASSED]
testScript                 [PASSED]
testEvalSHA                [PASSED]
testIntrospection          [PASSED]
testFailOver               [PASSED]
testRawCommand             [PASSED]
testMinimumVersion         [PASSED]
testPipelinePublish        [SKIPPED]
testBitsets                [PASSED]
testBitPos                 [PASSED]
test1000                   [PASSED]
testErr                    [PASSED]
testSet                    [PASSED]
testExtendedSet            [PASSED]
testGetSet                 [PASSED]
testRename                 [PASSED]
testRenameNx               [PASSED]
testMultiple               [PASSED]
testMultipleBin            [PASSED]
testSetTimeout             [PASSED]
testExpireAt               [PASSED]
testSetEx                  [PASSED]
testSetNX                  [PASSED]
testExpireAtWithLong       [PASSED]
testIncr                   [PASSED]
testIncrByFloat            [PASSED]
testDecr                   [PASSED]
testExists                 [PASSED]
testGetKeys                [PASSED]
testDelete                 [PASSED]
testType                   [PASSED]
testStr                    [PASSED]
testlPop                   [PASSED]
testrPop                   [PASSED]
testblockingPop            [PASSED]
testllen                   [PASSED]
testlPopx                  [PASSED]
testltrim                  [PASSED]
testlGet                   [PASSED]
testlrem                   [PASSED]
testsAdd                   [PASSED]
testscard                  [PASSED]
testsrem                   [PASSED]
testsMove                  [PASSED]
testsPop                   [PASSED]
testsRandMember            [PASSED]
testSRandMemberWithCount   [PASSED]
testsismember              [PASSED]
testsmembers               [PASSED]
testlSet                   [PASSED]
testsInter                 [PASSED]
testsInterStore            [PASSED]
testsUnion                 [PASSED]
testsUnionStore            [PASSED]
testsDiff                  [PASSED]
testsDiffStore             [PASSED]
testlrange                 [PASSED]
testttl                    [PASSED]
testPersist                [PASSED]
testMset                   [PASSED]
testRpopLpush              [PASSED]
testBRpopLpush             [PASSED]
testZAddFirstArg           [PASSED]
testZX                     [PASSED]
testHashes                 [PASSED]
testSetRange               [PASSED]
testObject                 [PASSED]
testMultiExec              [PASSED]
testPipeline               [SKIPPED]
testDifferentTypeString    [PASSED]
testDifferentTypeList      [PASSED]
testDifferentTypeSet       [PASSED]
testDifferentTypeSortedSet [PASSED]
testDifferentTypeHash      [PASSED]
testSerializerPHP          [PASSED]
testSerializerIGBinary     [PASSED]
testDumpRestore            [PASSED]
testGetLastError           [PASSED]
testEval                   [PASSED]
testSerialize              [PASSED]
testUnserialize            [PASSED]
testPrefix                 [PASSED]
testReadTimeoutOption      [PASSED]
testHScan                  [PASSED]
testSScan                  [PASSED]
testZScan                  [FAILED]
testPFCommands             [PASSED]

Skipped test: /root/phpredis/tests/RedisClusterTest.php:30 (testSortAsc)
Skipped test: /root/phpredis/tests/RedisClusterTest.php:31 (testSortDesc)
Skipped test: /root/phpredis/tests/RedisClusterTest.php:32 (testWait)
Skipped test: /root/phpredis/tests/RedisClusterTest.php:33 (testSelect)
Skipped test: /root/phpredis/tests/RedisClusterTest.php:34 (testReconnectSelect)
Skipped test: /root/phpredis/tests/RedisTest.php:68 (testPipelinePublish)
Skipped test: /root/phpredis/tests/RedisTest.php:2439 (testPipeline)

Assertion failed: /root/phpredis/tests/RedisClusterTest.php:78 (testRandomKey)
... zillion times

Assertion failed: /root/phpredis/tests/RedisTest.php:4674 (testZScan)

szepeviktor · 2016-02-05T18:28:29Z

Could we have another dirname() in tests/RedisClusterTest.php?

michael-grunder · 2016-02-05T18:37:46Z

OK well that looks much better. The problem with these kinds of bugs is that they are really unpredictable. Sometimes they break, sometimes not so much (and sometimes they change executable code, yay) 😃

I'll look into testZScan and testRandomKey to see if I can spot it. I will also run the tests on my mac and also on my CentOS VM.

edit: yes, I'll add dirname()

Jerry-Shaw · 2016-02-06T05:10:07Z

Hi, all, it is difficult for me to open my laptop when it's near Chinese Spring Festival. A lot of parties with friends and relatives.

As for dirname() path in the test's script, I always replace them to dirname(FILE) instead of $_SERVER['PHP_SELF'] before I run it in all the php scripts.

OK, I gotta go guys, and will test it later when I have my next chance to open my laptop.

yatsukhnenko · 2016-11-30T07:26:12Z

All tests passed

yatsukhnenko closed this as completed Nov 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Segfault in RedisArray test #727

Segfault in RedisArray test #727

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Segfault in RedisArray test #727

Segfault in RedisArray test #727

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!