Bug #49374 [Opn->Csd]: [PATCH] serialization adds random references

From: Date: Fri, 03 Jan 2020 10:21:14 +0000
Subject: Bug #49374 [Opn->Csd]: [PATCH] serialization adds random references
References: 1  Groups: php.bugs 
Request: Send a blank email to php-bugs+get-224681@lists.php.net to get a copy of this message
Edit report at https://bugs.php.net/bug.php?id=49374&edit=1

 ID:                 49374
 Updated by:         nikic@php.net
 Reported by:        wmeler at wp-sa dot pl
 Summary:            [PATCH] serialization adds random references
-Status:             Open
+Status:             Closed
 Type:               Bug
 Package:            Strings related
 Operating System:   *
 PHP Version:        5.*, 6
-Assigned To:        
+Assigned To:        nikic
 Block user comment: N
 Private report:     N

 New Comment:

This issue has been fixed in (IIRC) PHP 7.0, so closing here.

Link to the relevant code: https://github.com/php/php-src/blob/7ba18b0b392f668617f4cbb9e6129da831819bb4/ext/standard/var.c#L670


Previous Comments:
------------------------------------------------------------------------
[2014-08-08 02:02:50] stephen at heyday dot co dot nz

We ran into this issue yesterday using PHP 5.5.15 with mod_php under OSX 10.9. The issue was
consistently repeatable in our application environment with a serialize call involving only three
objects, I've tried to produce a standalone test case for this, but as pointed out by the
reporter it's extremely difficult demonstrate from PHP code as it depends on the memory
addresses of the zvals being serialized lining up perfectly.

Since this issue can occur by chance and corrupt data (though rarely), we are now using the igbinary
extension for serialization. The issue described above disappears when igbinary is enabled, and
returns when igbinary is disabled.

------------------------------------------------------------------------
[2010-04-01 14:43:12] grzegorz dot drozd at esky dot pl

Hello.

It seems to me that I just encountered this error. By implementing a new service which includes a
series (more than 70) objects each of which contains an array of objects I noticed the appearance of
random objects from another part of the data.
The data structure is as follows:

List
 item 1
 item 2
 item 3
   subitem 1
   subitem 2
   subitem 3
      element *
      element *
      element *

Objects marked with an asterisk appear in other places in this structure with a semi random
frequency. Serialized session has a size of about 2-3 MB.

All the elements are objects with very similar name:
List_item_subitem_element or List_item_subitem etc.

There are about 70 instances of item with 2-10 instances of subitem and 2-4 instances of element.

------------------------------------------------------------------------
[2009-12-29 19:31:22] jani@php.net

Never heard such thing as putting the patch as file somewhere where it can be downloaded from?
Don't be so arrogant, we might even commit your patches sometime.

------------------------------------------------------------------------
[2009-08-27 06:42:11] wmeler at wp-sa dot pl

I can't provide you simple script to reproduce the problem, as it depends on everything what is
on heap - environment variables, compilation, memory manager etc.
Why didn't I send following patch? because your bug reporting form has no such input ... You
also state: "Please do not insert any huge text here. Keep the description as short as
possible. If we want to get a strace, we will ask for it separately."
Feel free to change bug reporting form. I hope that patch will fit in comment...

--- ext/standard/var.c.orig	2009-08-27 09:18:53.628753000 +0200
+++ ext/standard/var.c	2009-08-27 09:39:06.932120000 +0200
@@ -470,25 +470,23 @@
 static inline int php_add_var_hash(HashTable *var_hash, zval *var, void *var_old TSRMLS_DC) /* {{{
*/
 {
 	ulong var_no;
-	char id[32], *p;
+	char id[32];
 	register int len;
 
-	/* relies on "(long)" being a perfect hash function for data pointers,
-	 * however the actual identity of an object has had to be determined
+	/* pointers to variables stored in binary hash keys
+	 * the actual identity of an object has had to be determined
 	 * by its object handle and the class entry since 5.0. */
 	if ((Z_TYPE_P(var) == IS_OBJECT) && Z_OBJ_HT_P(var)->get_class_entry) {
-		p = smart_str_print_long(id + sizeof(id) - 1,
-				(((size_t)Z_OBJCE_P(var) << 5)
-				| ((size_t)Z_OBJCE_P(var) >> (sizeof(long) * 8 - 5)))
-				+ (long) Z_OBJ_HANDLE_P(var));
-		*(--p) = 'O';
-		len = id + sizeof(id) - 1 - p;
+		zend_class_entry *ce = Z_OBJCE_P(var); 
+		memcpy(id,&ce,sizeof(void *));
+		memcpy(id+sizeof(void*),&(Z_OBJ_HANDLE_P(var)),sizeof(zend_object_handle));
+		len = sizeof(void*)+sizeof(zend_object_handle);
 	} else {
-		p = smart_str_print_long(id + sizeof(id) - 1, (long) var);
-		len = id + sizeof(id) - 1 - p;
+		memcpy(id,&var,sizeof(void *));
+		len = sizeof(void *);
 	}
 
-	if (var_old && zend_hash_find(var_hash, p, len, var_old) == SUCCESS) {
+	if (var_old && zend_hash_find(var_hash, id, len, var_old) == SUCCESS) {
 		if (!Z_ISREF_P(var)) {
 			/* we still need to bump up the counter, since non-refs will
 			 * be counted separately by unserializer */
@@ -500,7 +498,7 @@
 
 	/* +1 because otherwise hash will think we are trying to store NULL pointer */
 	var_no = zend_hash_num_elements(var_hash) + 1;
-	zend_hash_add(var_hash, p, len, &var_no, sizeof(var_no), NULL);
+	zend_hash_add(var_hash, id, len, &var_no, sizeof(var_no), NULL);
 	return SUCCESS;
 }
 /* }}} */

------------------------------------------------------------------------
[2009-08-26 15:03:57] jani@php.net

How about an example script to reproduce this and the patch? Why do we need to ask for the patch
separately anyway? Just show the patch or don't. Don't "ask to ask".

------------------------------------------------------------------------


The remainder of the comments for this report are too long. To view
the rest of the comments, please view the bug report online at

    https://bugs.php.net/bug.php?id=49374


--
Edit this bug report at https://bugs.php.net/bug.php?id=49374&edit=1


Thread (7 messages)

« previous php.bugs (#224681) next »