SERIALIZER_IGBINARY can cause issues with incr/decr #254

Closed
dgsan opened this Issue Sep 24, 2012 · 12 comments

Comments

Projects
None yet
6 participants

dgsan commented Sep 24, 2012

From phpredis:
$redis->set('bongo', 8);

From redis-cli: // I mean redis-cli... oops. (really late update 12-31-2012.)
get bongo
"\x00\x00\x00\x02\x06\b"

From phpredis:
$q = $redis->incr('bongo');
echo $q;
// 1 // Expecting: 9

From redis-cli:
get bongo
"1"

Expect: that you can set bongo to 8, call incr, get 9.

Most of the time the strings like "\x00\x00\x00\x02\x06\b" get treated as 0 when you call incr/decr, but once in a while they seem to trigger a segfault/core dump if you call get on them.

For example, I recently wound up with the value "\x00\x00\x00\x02\nP`\x97Q" (probably improperly formatted somehow), which causes get() of the key with that value to seg fault/core dump.

I'm kind of inclined to think that a mode where numeric values, or at least integer values, aren't serialized, but other types are, may be a good idea, as I'm not sure decr/incr could be made to work with serialized integers as they are atomic and thus probably assume/require redis's normal internal representation.

As it stands, working around this is presenting some challenges.

I'm checked out at commit 7dfac44.

dgsan commented Sep 24, 2012

My workaround is ending up much like various previous commenter's -- wrap the redis object in such a way that if(is_numeric) on set just store, if(is_numeric) on get just return, else use json encode,decode or some such (considering php serialize as not that portable).

Owner

michael-grunder commented Sep 25, 2012

Hey,

Check out the new branch json. Clone it like so:
git clone -b json https://github.com/nicolasff/phpredis.git

It's really experimental right now, as we'll have to figure out (a) if we want to incorporate JSON into the main branch, and (b) how to handle the whole associative array vs. stdClass decode option.

vsychov commented Apr 28, 2013

This also reproducible, with SERIALIZER_PHP

Owner

michael-grunder commented Apr 28, 2013

Hey,

I don't really think this is a bug. The purpose of the serializer is to allow the client to store whatever they want (arrays, objects, etc) in Redis in a transparent way. There is a non trivial amount of overhead in guessing if a value doesn't actually need to be serialized, and then more in determining if the value is just raw or has some sort of serialization error.

My suggestion would be to control this on the client side.

ahfeel commented Jun 3, 2016

Hello,

I'm bringing this issue back up as I think it's really a major issue with this library. Using a serializer totally demolishes the possibility of using incr/decr and this looks to me as a misconception.

I think there are three ways to deal and finally move on with this issue:

  • Or we make incr / decr atomic with the CAS commands
  • Or we just add an is_object check to know if we have to serialize values
  • Or we add new serialization consts (or an option ?) to enable this behavior.

I think it's about time to solve this big gotcha :)

I'm ok to propose a pull request if one of these alternative is ok for you @michael-grunder

Owner

michael-grunder commented Jun 20, 2016

Hey,

I still think this is something to be solved on the client side. In addition, I'm certain that any kind of global change to the behavior (e.g. not serializing numbers) would break tons of code.

For instance people might be using phpredis to store information serialized, and then consuming it from some other system, which expects serialized data. Adding another serialization option is a possibility I suppose (I don't know, SERIALIZE_IF_NOT_NUMBER), but it feels like adding something to the library for a very specific problem.

A simple client side solution would be something like:

$r = new Redis();
$r->connect('127.0.0.1', 6379);
$r->setOption(Redis::OPT_SERIALIZER, Redis::SERIALIZER_IGBINARY);

function mySetCommand($r, $key, $val) {
    if (is_numeric($val)) {
        return $r->rawCommand('set', $key, $val);
    } else {
        return $r->set($key, $val);
    }
}

mySetCommand($r, 'foo', new stdClass());
var_dump($r->get('foo'));
mySetCommand($r, 'foo', 'bar');
var_dump($r->get('foo'));
mySetCommand($r, 'foo', ['1', '2', '3']);
var_dump($r->get('foo'));

mySetCommand($r, 'foo', 8);
$r->incr('foo');
var_dump($r->get('foo'));

Which appears to work:

object(stdClass)#2 (0) {
}
string(3) "bar"
array(3) {
  [0]=>
  string(1) "1"
  [1]=>
  string(1) "2"
  [2]=>
  string(1) "3"
}
string(1) "9"

ahfeel commented Jun 20, 2016

Well, I understand your point about not breaking the world and that's exactly why in my previous proposal I said: "Or we add new serialization consts (or an option ?) to enable this behavior."

We could then provide something like
$r->setOption(Redis::OPT_SERIALIZE_NUMBERS, false);

This way we provide an official and not hacky way to have real support for numbers with incr/decr which are really important functionalities.

What do you think ?

Owner

michael-grunder commented Jun 21, 2016

I took a look at the code involved, it it looks like this could be done without causing a performance penalty (not a noticeable one anyway) during serialization.

We would need to handle explicit long and double values, as well as "numeric strings" if we wanted to be complete about it (php being so loosely typed). I suppose we could not worry about numeric strings, but if it's going into the library probably should.

Then, I suppose it's just a matter of which methods this would apply to. Does it apply to every method, or only set and hset/hmset (e.g. the types you can run increment/decrement against).

ahfeel commented Jul 5, 2016

I think it should apply only to set / hset / hmset, it will be enough to get the desired functionality without altering any other method. What do you think ?

Hint-ru commented May 13, 2017 edited

I think, json serializer is easy way to solve the issue, because encoded number equals to decoded number.
Add JSON serializer: #246

Member

yatsukhnenko commented Jun 20, 2017

Don't use igbinary serializer to avoid problems with incr/decr or don't use incr/decr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment