Home Original page

incorrect processing of numerical string keys of array in arbitrary serial. data

Bug #43614 incorrect processing of numerical string keys of array in arbitrary serial. data
Submitted: 2007-12-17 08:14 UTC Modified: 2008-03-19 03:02 UTC
Votes:19
Avg. Score:4.8 ± 0.4
Reproduced:17 of 18 (94.4%)
Same Version:13 (76.5%)
Same OS:6 (35.3%)
From: dmitriy dot buldakov at toatech dot com Assigned:
Status: Closed Package: Arrays related
PHP Version: 5.2.5 OS: Mac OS X
Private report: No CVE-ID: None

 [2007-12-17 08:14 UTC] dmitriy dot buldakov at toatech dot com

Description:
------------
php converts numerical string in array keys to integer if possible.

php4 converts those string while unserialize as well as while array processing of php code

php5 converts those string while array processing of php code only, and not converts while the unserialize a string

As the result - unserialized array can contains fake elements.
For example - there is no way to get access to the element by a key.

Reproduce code:
---------------
<?php 
 
$a = unserialize('a:2:{s:2:"10";i:1;s:2:"01";i:2;}'); 
 
print $a['10']."\n";

$a['10'] = 3; 
$a['01'] = 4; 

print_r($a);

foreach ($a as $k => $v) 
{ 
  print 'KEY: '; 
  var_dump($k); 
  print 'VAL: '; 
  var_dump($v); 
  print "\n"; 
} 
 
?>

Expected result:
----------------
Dmitriy-Buldakov:~ dmitry$ php4 test.php
1
Array
(
    [10] => 3
    [01] => 4
)
KEY: int(10)
VAL: int(3)
 
KEY: string(2) "01"
VAL: int(4)
 
Dmitriy-Buldakov:~ dmitry$


Actual result:
--------------
Dmitriy-Buldakov:~ dmitry$ php5 test.php
PHP Notice:  Undefined index:  10 in /Users/dmitry/test.php on line 5
 
Notice: Undefined index:  10 in /Users/dmitry/test.php on line 5
 
Array
(
    [10] => 1
    [01] => 4
    [10] => 3
)
KEY: string(2) "10"
VAL: int(1)
 
KEY: string(2) "01"
VAL: int(4)
 
KEY: int(10)
VAL: int(3)

Dmitriy-Buldakov:~ dmitry$

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports

 [2007-12-17 12:00 UTC] felipe@php.net

Hmm, ok, arbitray string...

Simple solution:

Index: var_unserializer.c
===================================================================
RCS file: /repository/php-src/ext/standard/var_unserializer.c,v
retrieving revision 1.70.2.4.2.7.2.3
diff -u -u -r1.70.2.4.2.7.2.3 var_unserializer.c
--- var_unserializer.c  17 Oct 2007 10:36:33 -0000      1.70.2.4.2.7.2.3
+++ var_unserializer.c  17 Dec 2007 11:58:43 -0000
@@ -282,6 +282,10 @@
                        return 0;
                }
 
+               if (Z_TYPE_P(key) == IS_STRING && strspn(Z_STRVAL_P(key), "0123456789") == Z_STRLEN_P(key)) {
+                       convert_to_long(key);
+               }
+
                switch (Z_TYPE_P(key)) {
                        case IS_LONG:
                                if (zend_hash_index_find(ht, Z_LVAL_P(key), (void **)&old_data)==SUCCESS) {

 [2007-12-17 13:38 UTC] dmitriy dot buldakov at toatech dot com

Looks like the simple solution is not very good.
As far as I understood the patch just convert any numeric string to long.

It is not right way because:
1) it dose not make range check ("9999999999" is converted to 2147483647)
2) it dose not recognize negative values ("-10" is not converted to string)
3) it ignores leading zero ("01" to 1)

For example, the reproduce code result is still not expected:

1
Array
(
   [10] => 3
   [1] => 2
   [01] => 4
)
KEY: int(10)
VAL: int(3)

KEY: int(1)
VAL: int(2)

KEY: string(2) "01"
VAL: int(4)

 [2007-12-17 16:03 UTC] dmitriy dot buldakov at toatech dot com

Last patch works much more better, but there is still a problem.

the patch ignores leading spaces.

for example keys ' 10' and ' -10' are converted to integer 10 and -10.

 [2007-12-18 12:04 UTC] dmitriy dot buldakov at toatech dot com

I have not tesdet last patch yet, but looks like "0" is not converted to int 0 with the patch.

 [2007-12-18 13:46 UTC] dmitriy dot buldakov at toatech dot com

The following code works well


		switch (Z_TYPE_P(key)) {
			case IS_LONG:
				zend_hash_index_update(ht, Z_LVAL_P(key), &data, sizeof(data), NULL);
				break;
			case IS_STRING:
				zend_symtable_update(ht, Z_STRVAL_P(key), Z_STRLEN_P(key) + 1, &data, sizeof(data), NULL);
				break;
		}

but looks like still there is a problem here.
compearing var_unserialize.c with array.c you can see that key array.c uses more sufficient key preparation.

So, about the code - what should I do to put the code into repository?

 [2007-12-18 13:49 UTC] dmitriy dot buldakov at toatech dot com

--- var_unserializer.c.orig       2007-12-18 12:13:16.000000000 +0200
+++ var_unserializer.c  2007-12-18 15:40:22.000000000 +0200
@@ -282,16 +282,10 @@ static inline int process_nested_data(UN
 
                switch (Z_TYPE_P(key)) {
                        case IS_LONG:
-                               if (zend_hash_index_find(ht, Z_LVAL_P(key), (void **)&old_data)==SUCCESS) {
-                                       var_push_dtor(var_hash, old_data);
-                               }
                                zend_hash_index_update(ht, Z_LVAL_P(key), &data, sizeof(data), NULL);
                                break;
                        case IS_STRING:
-                               if (zend_hash_find(ht, Z_STRVAL_P(key), Z_STRLEN_P(key) + 1, (void **)&old_data)==SUCCESS) {
-                                       var_push_dtor(var_hash, old_data);
-                               }
-                               zend_hash_update(ht, Z_STRVAL_P(key), Z_STRLEN_P(key) + 1, &data, sizeof(data), NULL);
+                               zend_symtable_update(ht, Z_STRVAL_P(key), Z_STRLEN_P(key) + 1, &data, sizeof(data), NULL);
                                break;
                }

 [2007-12-18 14:09 UTC] felipe@php.net

No, it works fine with '0'.
I improved the code and test now.

Results:

array(1) {
  [999999999]=>
  int(1)
}
array(1) {
  ["9999999999"]=>
  int(1)
}
array(1) {
  ["+1"]=>
  int(1)
}
array(1) {
  [11]=>
  int(1)
}
array(1) {
  ["00"]=>
  int(1)
}
array(1) {
  [0]=>
  int(1)
}
array(1) {
  ["-0"]=>
  int(1)
}
array(1) {
  ["-01"]=>
  int(1)
}
array(1) {
  [-10]=>
  int(1)
}

 [2007-12-18 14:18 UTC] dmitriy dot buldakov at toatech dot com

final version of the patch

--- var_unserializer.c  2007-12-18 16:11:48.000000000 +0200
+++ var_unserializer.c.old      2007-12-18 16:11:32.000000000 +0200
@@ -288,10 +288,10 @@ static inline int process_nested_data(UN
                                zend_hash_index_update(ht, Z_LVAL_P(key), &data, sizeof(data), NULL);
                                break;
                        case IS_STRING:
-                               if (zend_hash_find(ht, Z_STRVAL_P(key), Z_STRLEN_P(key) + 1, (void **)&old_data)==SUCCESS) {
+                               if (zend_symtable_find(ht, Z_STRVAL_P(key), Z_STRLEN_P(key) + 1, (void **)&old_data)==SUCCESS) {
                                        var_push_dtor(var_hash, old_data);
                                }
-                               zend_hash_update(ht, Z_STRVAL_P(key), Z_STRLEN_P(key) + 1, &data, sizeof(data), NULL);
+                               zend_symtable_update(ht, Z_STRVAL_P(key), Z_STRLEN_P(key) + 1, &data, sizeof(data), NULL);
                                break;
                }

 [2007-12-18 14:38 UTC] dmitriy dot buldakov at toatech dot com

Felipe, yes, you are right, yours code works good with "0" but still have problem with leading spaces. I mean that unserialize with yours patch converts string(" 9") to int(9).

 [2008-03-19 03:02 UTC] felipe@php.net

This bug has been fixed in CVS.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.
 
Thank you for the report, and for helping us make PHP better.

Thanks for the patch.