php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
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
View Add Comment Developer Edit
Anyone can comment on a bug. Have a simpler test case? Does it work for you on a different platform? Let us know!
Just going to say 'Me too!'? Don't clutter the database with that please !
Your email address:
MUST BE VALID
Solve the problem:
15 - 12 = ?
Subscribe to this entry?

 
 [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

Add a Patch

Pull Requests

Add a Pull Request

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 13:54 UTC] felipe@php.net
Yes, correct. I've used not suitable function. Tony alerted me of a correct function.

http://ecl.zoone.com.br/etc/patches/bug43614.patch
http://ecl.zoone.com.br/etc/patches/bug43614.phpt
 [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 01:52 UTC] felipe@php.net
Updated. The function did accept other characters. And how not is possible to use de HANDLE_NUMERIC(), I make a loop and check for each char.

http://ecl.zoone.com.br/etc/patches/bug43614.patch

 [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.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Fri Mar 29 06:01:29 2024 UTC