Before unserializing a Redis value with igbinary, check if it actually contains the correct header #761

mcuelenaere · 2016-03-15T17:23:31Z

The bug can be reproduced with this script:

<?php
$r = new Redis();
$r->connect('localhost', 6379);
$r->setOption(Redis::OPT_SERIALIZER, Redis::SERIALIZER_IGBINARY);

for ($i=0; $i < 1000; $i++) {
  $r->incr('a', -1);
}
var_dump($r->get('a'));
$r->del('a');

and will result in this output:

PHP Warning:  igbinary_unserialize_header: unsupported version: 758198320, should be 1 or 2 in /Users/mcuelenaere/Projects/phpredis/test.php on line 10
PHP Stack trace:
PHP   1. {main}() /Users/mcuelenaere/Projects/phpredis/test.php:0
PHP   2. Redis->get() /Users/mcuelenaere/Projects/phpredis/test.php:10

Warning: igbinary_unserialize_header: unsupported version: 758198320, should be 1 or 2 in /Users/mcuelenaere/Projects/phpredis/test.php on line 10

Call Stack:
    0.0007     232768   1. {main}() /Users/mcuelenaere/Projects/phpredis/test.php:0
    0.0404     242624   2. Redis->get() /Users/mcuelenaere/Projects/phpredis/test.php:10

string(5) "-1000"


        Before unserializing a Redis value with igbinary, check if it actuall…

…y contains the correct header

mcuelenaere · 2016-03-15T17:24:41Z

This should fix #387, #301 and #39.

michael-grunder · 2016-03-15T17:32:51Z

Thanks for the merge request. Is that header an 'always and forever' type thing, or could it pose problems depending on the version of igbinary?

Cheers
mike

mcuelenaere · 2016-03-15T20:04:06Z

As I couldn't find any documentation on the igbinary protocol, I based myself on the igbinary source.
More specifically here and here.

I'm not sure if this is an 'always and forever' thing, but so far there seem to have been only 2 versions of the igbinary protocol and they start with a 0x1 and 0x2 header respectively.

An alternative way would be igbinary exposing an extra function (eg bool igbinary_is_valid(const char* buf, size_t len)) that would be called instead of doing the validation inline here.

michael-grunder · 2016-03-15T20:16:47Z

An alternative way would be igbinary exposing an extra function (eg bool igbinary_is_valid(const char* buf, size_t len)) that would be called instead of doing the validation inline here.

That would seem almost too logical 😃

I don't have a problem inlining the validation like this but was just wondering if we would run into versioning problems for ancient versions of igbinary. I'll pull it down and give it a test and then get it merged for you.

Thanks again!
Mike


        Not all versions contain the trailing NULL byte, so do not require it

mcuelenaere · 2016-03-16T09:26:43Z

I changed the val_len check to not include the NULL byte, as in the first commit it wasn't being written.

michael-grunder · 2016-03-31T14:14:20Z

I fetched your repo and have merged this into the develop branch locally so I'll close this request. Just going to do a few tests and then you'll see it on the next push.

Then I have to do all of it again for php7 😃

Before unserializing a Redis value with igbinary, check if it actuall…

3266b22

…y contains the correct header

Not all versions contain the trailing NULL byte, so do not require it

5528297

michael-grunder closed this Mar 31, 2016

TysonAndre mentioned this pull request Nov 8, 2016

php7.0.11 bug with PHPRedis: igbinary_unserialize_header: unsupported version: "Arra" igbinary/igbinary#98

Closed

michael-grunder mentioned this pull request Aug 29, 2017

config('GET', '*') with igbinary creates a warning #301

Closed

phpredis / phpredis

Before unserializing a Redis value with igbinary, check if it actually contains the correct header #761

Before unserializing a Redis value with igbinary, check if it actually contains the correct header #761

mcuelenaere commented Mar 15, 2016

mcuelenaere commented Mar 15, 2016

michael-grunder commented Mar 15, 2016

mcuelenaere commented Mar 15, 2016

michael-grunder commented Mar 15, 2016

mcuelenaere commented Mar 16, 2016

michael-grunder commented Mar 31, 2016

phpredis / phpredis

Sponsor phpredis/phpredis

Join GitHub today

Before unserializing a Redis value with igbinary, check if it actually contains the correct header #761

Before unserializing a Redis value with igbinary, check if it actually contains the correct header #761

Conversation

mcuelenaere commented Mar 15, 2016

mcuelenaere commented Mar 15, 2016

michael-grunder commented Mar 15, 2016

mcuelenaere commented Mar 15, 2016

michael-grunder commented Mar 15, 2016

mcuelenaere commented Mar 16, 2016

michael-grunder commented Mar 31, 2016