Description

ssigwart

opened on Oct 24, 2018

Issue body actions

Environment

• OS: Amazon Linux 2
• PHP: 7.2.8
• Igbinary: 2.0.7

How to Reproduce

You can run this on Amazon Linux 2:

amazon-linux-extras install -y php7.2
yum -y install php-pecl-igbinary

cat <<'EOF' > test.php
<?php
$data = igbinary_unserialize(base64_decode('AAAAAhQFEQFhJREQCqtplnlxQ4qR3iK8rZtdkBECaXAlERAAAAAAAAAAAAAAAAAAAAABEQFzJQURAXQKW8/W5REBdiUKW9B/pQ=='));
echo 'Igbinary: ' . md5(igbinary_serialize($data)) . "\n";
echo 'PHP: ' . md5(serialize($data)) . "\n";
if ($data['v'] == 'X')
	echo "here\n";
echo 'Igbinary: ' . md5(igbinary_serialize($data)) . "\n";
echo 'PHP: ' . md5(serialize($data)) . "\n";
EOF

php test.php

Expected Results

Igbinary serialization should not change.

Actual Results

Igbinary: 25bb60cf13c1877b6969069d46394639
PHP: 9a9b1dbdd80d68a819ff36ef4edae47c
Igbinary: f445e038c94109bae4d7fdbbaea5f83f
PHP: 9a9b1dbdd80d68a819ff36ef4edae47c

Activity

1. TysonAndre

   added

   question

   on Oct 24, 2018
2. TysonAndre commented on Oct 24, 2018

   

   TysonAndre

   on Oct 24, 2018

   Member

   More actions

   I'm able to reproduce this in php 7.2 - not in 7.3.

   It seems that the original serialized data from your example comes from a PHP array which had many references to its elements (i.e. &$arr[$field]). Those references included a single integer (0x5bd07fa5) that was referenced once within the data structure passed to igbinary_serialize originally, but one or more times in other datastructures.

   • When the php interpreter is accessing the reference, it sees that it's a value with exactly one reference. Because it's safe to do so (it wouldn't be if there were 2+ references), the php interpreter (at least for php 7.2) converts the reference to a non-reference during the variable access, so the serialization removes the byte 0x25 (igbinary_type_ref).
   • igbinary is able to unserialize both of these values without any errors, so I don't consider this to be a problem.
   • one approach to avoid unexpected behavior on re-serialization might be to normalize references during unserialization.

   An explanation of what the hexadecimal contents of igbinary_serialize are:

   (see https://github.com/igbinary/igbinary/blob/2.0.8/src/php7/igbinary.c#L73-L124)

   Comparing bin2hex(igbinary_serialize()) before and after the access:
   0000000214051101612511100aab69967971438a91de22bcad9b5d90110269702511100000000000000000000000000000000111017325051101740a5bcfd6e5110176250a5bd07fa5
   0000000214051101612511100aab69967971438a91de22bcad9b5d90110269702511100000000000000000000000000000000111017325051101740a5bcfd6e51101760a5bd07fa5
   
   ------------------------
   00000002  // igbinary version header
   14  05  // 5-element array
     1101 61  // key "a"
       25 11 10 0aab69967971438a91de22bcad9b5d90 // serialization of PHP reference (i.e. &$x) to the 16-byte string hex2bin('0aab69967971438a91de22bcad9b5d90') - igbinary_type_ref == 25
     1102 6970  // 2-byte string "ip"
       25 11 10 00000000000000000000000000000001 // another reference
     1101 73  // "s"
       25 05  // reference to boolean true 
     1101 74  // "t"
       0a 5bcfd6e5 // int(1540347621)
     1101 76  // "v"
       25 0a 5bd07fa5 // reference to int(1540390821)
   ------------------------
   
   0000000214051101612511100aab69967971438a91de22bcad9b5d90110269702511100000000000000000000000000000000111017325051101740a5bcfd6e51101760a5bd07fa5 split up
   
   ------------------------
   00000002
   14  05  // 5-element array
     1101 61  // a
       25 11 10 0aab69967971438a91de22bcad9b5d90 // serialization of PHP reference (i.e. &$x) to the 16-byte string hex2bin('0aab69967971438a91de22bcad9b5d90') - igbinary_type_ref == 25
     1102 6970  // 2-byte string "ip"
       25 11 10 00000000000000000000000000000001 // another reference
     1101 73  // "s"
       25 05  // reference to boolean true 
     1101 74  // "t"
       0a 5bcfd6e5 // int(1540347621)
     1101 76  // "v"
       0a 5bd07fa5 // int(1540390821) - not a reference
   ------------------------
   
3. TysonAndre

   changed the title [-]Data Serialization Changes after Variable Access[/-] [+]Data Serialization Changes after Variable Access for an array of serialized references to variables[/+] on Oct 24, 2018
4. ssigwart commented on Oct 25, 2018

   

   ssigwart

   on Oct 25, 2018

   Author

   More actions

   Thank you for the detailed explanation. I was able to track it down to array_walk_recursive and created this simple demo of the issue.

   $item = ['a' => 1];
   
   // Convert data types
   array_walk_recursive($item, function(&$val) {
   	if (is_string($val))
   		$val = (int)$val;
   });
   
   $b64 = base64_encode(igbinary_serialize($item));
   $var = igbinary_unserialize(base64_decode($b64));
   echo md5(igbinary_serialize($var)) . "\n";
   if ($var['a'] == 'X')
   	echo "here\n";
   echo md5(igbinary_serialize($var)) . "\n";

   You suggested that I normalize references during unserialization. I'm not exactly sure how I'd do that. However, I don't think it would help my use case. I ran across the issue when using phpredis with igbinary as a serializer. I'm storing data in a sorted set in Redis. At some point, I fetch results from the set and do a little processing. If a result is old, I remove it from the set. That's where the problem comes in. I tell Redis to remove the item, but since it serializes differently, it can't find the item to remove. Do you have any suggestions on how to resolve that? The only thought I have is to serialize, then unserialize any variable before sending it to Redis, but that seems a bit inefficient. Also, is this something where phpredis might need to update the way it's using igbinary?
5. TysonAndre commented on Oct 25, 2018

   

   TysonAndre

   on Oct 25, 2018

   Member

   More actions

   You suggested that I normalize references during unserialization.

   I suggested that igbinary might do that during unserialization, not the PHP code using igbinary. If the reference count was 1 after unserialization, a reference group could be normalized to a non-reference.

   A possible workaround: If you could avoid using references before serializing (e.g. use keys instead to refer to array elements), that might be one way, but is probably impractical for your use case, and wouldn't help with old data.

   Also, is this something where phpredis might need to update the way it's using igbinary?

   No, it isn't something specific to phpredis

   ---

   Aside (For future reference): It seems like serialize() takes a different approach to references than igbinary_serialize() - The first time it sees a value (not a key), it tracks that value as something that could become a reference. The second time, it emits a reference to the index of that value (R:2). So, I think it's able to make any value a reference retroactively

   Unfortunately, I can't really change the way references are represented (i.e. using 0x25 for the first reference to a value) in version 2 (0x00000002) of igbinary, as older installations would be unable to unserialize data)

   • and changing a reference to a non-reference would decrease subsequent reference numbers by 1, making this much more complicated than deleting the byte 0x25 from the serialization when there is more data

   php > $x = ['key' => &$y, 'other' => &$y];
   php > var_export(serialize($x));
   'a:2:{s:3:"key";i:2;s:5:"other";R:2;}'
   php > $z = 5; $x = ['unrelated' => null, 'key' => &$y, 'other' => &$y];
   // adding a value before the first $y changes R:2 to R:3
   php > var_export(serialize($x));
   'a:3:{s:9:"unrelated";N;s:3:"key";i:2;s:5:"other";R:3;}'
   
6. ssigwart commented on Oct 25, 2018

   

   ssigwart

   on Oct 25, 2018

   Author

   More actions

   Ok. Thanks. Using the PHP serializer solves my use case as it's just a small project. Another option is to JSON encode data before adding it to redis sets so that I can still use igbinary.

Issue actions