php5-fpm segfault RedisArray #350

Closed
egorovrv opened this Issue Jul 9, 2013 · 12 comments

Projects

None yet

3 participants

@egorovrv
egorovrv commented Jul 9, 2013

$this->redis = new RedisArray(array("localhost:63791",));
wrong port
$this->redis->get("user2:name"); it's ok RedisException

if use this
$this->redis->mGet(array ( 0 => 'xxxx', ));

No RedisException
and
php5-fpm[6214]: segfault at 4 ip 00000000006d3980 s

Redis Version 2.2.3
Description: Debian GNU/Linux 6.0.7 (squeeze)
PHP 5.3.19

What to do ?

@egorovrv
egorovrv commented Aug 6, 2013

Nobody have this error ?

@michael-grunder
Member

Hey there,

Yes, so I actually did encounter this error when adding the ability to use integer keys with mGet. I think that the changes fixed it.

Right now it's living in the branch feature/ra_mset_intkeys if you would like to try it out. We're going to merge this and a few other changes into develop and then master in the near term if you wanted to wait for that.

Cheers,
Mike

@egorovrv
egorovrv commented Aug 6, 2013

Hi Mike.
I try to use # On branch feature/ra_mset_intkeys
Have the same error.
[22470.658514] php5-fpm[1166]: segfault at 4 ip 00000000006d3980 sp 00007ffffdfc5748 error 4 in php5-fpm[400000+826000]
Segfault in
zend_hash_quick_find(Z_ARRVAL_P(z_ret), NULL, 0, j, (void**)&z_cur);

It's not good. Have any idea ?

@michael-grunder
Member

I will attempt to replicate this locally and fix it for you. The current master branch doesn't actually support integer keys for mset or mget within RedisArray. The segfault occurs (at least when I was testing the new branch) when you only pass integer keys (tries to access/free a null pointer).

I'll send an update later today

@egorovrv
egorovrv commented Aug 6, 2013

Thank you, I think it's very important for everyone.

@michael-grunder
Member

Hey, I'm an idiot 😃

I didn't notice that you were connecting to an invalid port, but had focused on the fact that you were trying to use an integer key in the context of mset/mget.

I too can get it to segfault when I pass in an invalid port, which means that I can fix it!

Cheers,
Mike

@michael-grunder
Member

OK, I have found the problem. The question though, is what the proper solution is in this case.

The way RedisArray handles commands like MGET and MSET is to forward the call to the phpredis object. In cases where it can't communicate with the redis server, NULL is being returned. The segfault is occurring because the handler command is assuming that it will get back an array.

I think the proper solution would be for the whole command to fail, and an exception thrown in this case, rather than only returning keys that are able to be retrieved (partial success). Otherwise, you might not know that a failure occured.

@nicolasff What do you think?

Cheers,
Mike

@nicolasff
Contributor

@michael-grunder isn't an exception set when that happens? We could check that we got a NULL and return early from that function, leaving the PHP code to deal with the exception.

@michael-grunder
Member

I'm actually not seeing an exception set. I was also expecting to see an exception in this case. What's happening is that it's failing on the socket write, not the socket get.

I will look more closely to see why this is. Either way, I think a full fail is the right way to go with an exception thrown.

@michael-grunder
Member

Sorry, I wasn't paying attention. I bet there is an exception being set, but the segfault is preventing it from being thrown. Will sort the NULL check and push a hotfix 😃

@michael-grunder michael-grunder added a commit that referenced this issue Aug 6, 2013
@michael-grunder michael-grunder Make sure RedisArray::mget returns an array
Add a check in mget to make sure that the forwarded call is
returning with an array, and not NULL or something else.

Addresses issue #350
6418410
@michael-grunder
Member

@egorovrv can you please try it now. It's merged into master.

@nicolasff Sorry for being retarded 😃

Cheers!
Mike

@egorovrv
egorovrv commented Aug 7, 2013

Thanks michael-grunder.
it's good.
RedisException Connection closed

It was better to write Connection timeout. Program can choose another server.
But i read this is another feature.

@egorovrv egorovrv closed this Aug 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment