msgpack / msgpack-php

msgpack.org[PHP]
BSD 3-Clause "New" or "Revised" License
775 stars 120 forks source link

Object property serialization order #60

Closed rybakit closed 9 years ago

rybakit commented 9 years ago

I run into a problem while trying to deserealize data which was serialized using Lua. Here is a simplified example:

po = []
po[msgpack.NULL] = 'stdClass'
po['foo'] = 'bar'
packed = msgpack.encode(po)

As Lua doesn't support ordered hash tables, po keys can be in any order, and most of the time I get {foo: 'bar', "\0": 'stdClass'} = 0x82a3666f6fa3626172c0a8737464436c617373.

Php msgpack can't deserialized this data correctly, probably because it expects "\0": 'stdClass' to be the first item in the array, i.e.: 0x82c0a8737464436c617373a3666f6fa3626172.

So, the question, could this extension be enhanced to not depend on the position of the "\0" key?

Sean-Der commented 9 years ago

Hey @rybakit

So AFAIK the zend engine API shouldn't care about order. While they are stored as a list, I do not think the position+key matters. However, zero length keys does seem edge casey to me (I know we do have tests for this though)

I am debugging this now, and will update it when I know more.


What value are you expecting back after deserialization? If you are encoding/decoding in multiple languages you should also be setting PHPONLY to false, then you will end up with the following

$msgpack = new MessagePack();
$msgpack->setOption(MessagePack::OPT_PHPONLY, false);
var_dump($msgpack->unpack( hex2bin('82c0a8737464436c617373a3666f6fa3626172')));
var_dump($msgpack->unpack( hex2bin('82a3666f6fa3626172c0a8737464436c617373')));

----

Warning: [msgpack] (msgpack_unserialize_map_item) illegal key type in /home/vagrant/code/msgpack-php/test.php on line 5
array(2) {
  [""]=>
  string(8) "stdClass"
  ["foo"]=>
  string(3) "bar"
}

Warning: [msgpack] (msgpack_unserialize_map_item) illegal key type in /home/vagrant/code/msgpack-php/test.php on line 6
array(2) {
  ["foo"]=>
  string(3) "bar"
  [""]=>
  string(8) "stdClass"
}

Or are you looking to create PHP objects via input crafted from Lua? While possible, that is certainly an undefined behavior. Looking at the code it could be done, but would be expensive once you hit the null key (containing the class name) you would then have to copy your half serialized object to the new object. If you are really stuck I can look into making this work for you, but you are depending on funky behavior.

rybakit commented 9 years ago

Hey @Sean-Der, thanks for the quick response. I'm trying to get back a php object. The combo looks like this:

$packer = new MessagePack(true);
$packed = $packer->pack((object) ['foo' => 'bar']); 
// 0x82c0a8737464436c617373a3666f6fa3626172

Then the packed data goes through Lua:

unpacked = msgpack.decode(packed)
packed = msgpack.encode(unpacked) 
-- 0x82a3666f6fa3626172c0a8737464436c617373

And returns to php:

$obj = $packer->unpack($packed);
var_dump($obj);
// class stdClass#3 (0) {}
rybakit commented 9 years ago

would be expensive once you hit the null key (containing the class name) you would then have to copy your half serialized object to the new object.

What if you serialize an object into indexed array ['stdClass', ['foo' => 'bar']]? This will guarantee the same order across different languages, but introduce a BC break and I don’t know if it’s worth it. Or store the data into a temporary structure before building an object?

If you are really stuck I can look into making this work for you, but you are depending on funky behavior.

I try to figure out first what is possible in my case. If you say that it will be funky to support the object serialization, then I will probably just drop supporting objects in my code.

Sean-Der commented 9 years ago

Hey @rybakit try 488779bc2c62a210d5731cfce9963c7e5654e8c3 for PHP5 and 116cc56af9baa9ce7b48538da365fc7b14721035 for PHP7

rybakit commented 9 years ago

Works like a charm :+1: Thank you!

Sean-Der commented 9 years ago

Awesome! Now I just need some tests. Hand crafting these would not be fun, is there anything special you did to make your test values? I just want to make test values for

thanks!

rybakit commented 9 years ago

@Sean-Der Please check https://github.com/msgpack/msgpack-php/pull/61.

Sean-Der commented 9 years ago

Thank you so much @rybakit , I owe you big time :+1: I am gonna go grok the tests and the merge them thank you so much!

rybakit commented 9 years ago

Happy to help, and thank you too ;)

rybakit commented 8 years ago

@Sean-Der @laruence Looks like the fix for this issue is not present in the latest versions (neither 0.5.7 nor 2.0.0). Do you plan to create a new tag with the fix anytime soon?