Merge pull request #190 from TysonAndre/fix-custom-serializer

Fix memory leak/failed check for duplicates when serializing arrays and object

apc_cache.c

@@ -1234,7 +1234,10 @@ static zval* my_serialize_object(zval* dst, const zval* src, apc_context_t* ctxt
 		}
 
 		ZVAL_STR(dst, serial);
-		Z_TYPE_INFO_P(dst) = IS_OBJECT;
+		/* Give this the type of an object/array, and the same type flags as a string. */
+		/* The only necessary one is probably IS_TYPE_REFCOUNTED - (We check Z_REFCOUNTED_P when de-duplicating serialized values. */
+		/* Probably safe - When copying strings into shared memory, the code gives it type IS_STRING_EX, */
+		Z_TYPE_INFO_P(dst) = Z_TYPE_P(src) | ((IS_TYPE_REFCOUNTED | IS_TYPE_COPYABLE) << Z_TYPE_FLAGS_SHIFT);
 		efree(buf);
     }
 
@@ -1534,15 +1537,15 @@ static APC_HOTSPOT zend_reference* my_copy_reference(const zend_reference* src,
 }
 
 /* {{{ my_copy_zval */
+/* This function initializes *dst (temporary zval with request lifetime) with the (possibly serialized) contents of the serialized value in *src */
 static APC_HOTSPOT zval* my_copy_zval(zval* dst, const zval* src, apc_context_t* ctxt)
 {
     apc_pool* pool = ctxt->pool;
 
     assert(dst != NULL);
     assert(src != NULL);
 
-	memcpy(dst, src, sizeof(zval));
-
+	/* If src was already unserialized, then make dst a copy of the unserialization of src */
 	if (Z_REFCOUNTED_P(src)) {
 		if (zend_hash_num_elements(&ctxt->copied)) {
             zval *rc = zend_hash_index_find(
@@ -1555,13 +1558,17 @@ static APC_HOTSPOT zval* my_copy_zval(zval* dst, const zval* src, apc_context_t*
 		}
 	}
 
+	/* Copy the raw bytes of the type, value, and type flags */
+	memcpy(dst, src, sizeof(zval));
+
     switch (Z_TYPE_P(src)) {
     case IS_RESOURCE:
     case IS_TRUE:
     case IS_FALSE:
     case IS_LONG:
     case IS_DOUBLE:
     case IS_NULL:
+        /* No additional work for scalars, they aren't ref counted */
         break;
 
 	case IS_REFERENCE:
@@ -1597,6 +1604,8 @@ static APC_HOTSPOT zval* my_copy_zval(zval* dst, const zval* src, apc_context_t*
 
     case IS_OBJECT:
         if (ctxt->copy == APC_COPY_IN) {
+            /* For objects and arrays, their pointer points to a serialized string instead of a zend_array or zend_object. */
+            /* Unserialize that, and on success, give dst the default type flags for an object/array (We check Z_REFCOUNTED_P below). */
             dst = my_serialize_object(dst, src, ctxt);
         } else
             dst = my_unserialize_object(dst, src, ctxt);

tests/apc_005b.phpt

@@ -0,0 +1,51 @@
+--TEST--
+APC: apcu_store/fetch with arrays with duplicate object
+--SKIPIF--
+<?php require_once(dirname(__FILE__) . '/skipif.inc'); ?>
+--INI--
+apc.enabled=1
+apc.enable_cli=1
+apc.file_update_protection=0
+--FILE--
+<?php
+
+$o = new stdClass();
+$foo = array($o, $o);
+
+var_dump($foo);
+
+apcu_store('foo',$foo);
+
+$bar = apcu_fetch('foo');
+var_dump($foo);
+// $bar[0] should be identical to $bar[1], and not a reference
+var_dump($bar);
+?>
+===DONE===
+<?php exit(0); ?>
+--EXPECTF--
+array(2) {
+  [0]=>
+  object(stdClass)#1 (0) {
+  }
+  [1]=>
+  object(stdClass)#1 (0) {
+  }
+}
+array(2) {
+  [0]=>
+  object(stdClass)#1 (0) {
+  }
+  [1]=>
+  object(stdClass)#1 (0) {
+  }
+}
+array(2) {
+  [0]=>
+  object(stdClass)#2 (0) {
+  }
+  [1]=>
+  object(stdClass)#2 (0) {
+  }
+}
+===DONE===

tests/apc_005c.phpt

@@ -0,0 +1,59 @@
+--TEST--
+APC: apcu_store/fetch with arrays with two object references
+--SKIPIF--
+<?php require_once(dirname(__FILE__) . '/skipif.inc'); ?>
+--INI--
+apc.enabled=1
+apc.enable_cli=1
+apc.file_update_protection=0
+--FILE--
+<?php
+
+$o = new stdClass();
+$foo = array(&$o, &$o);
+
+var_dump($foo);
+
+apcu_store('foo',$foo);
+
+$bar = apcu_fetch('foo');
+var_dump($foo);
+// $bar[0] should be a reference to $bar[1].
+var_dump($bar);
+$bar[0] = 'roh';
+var_dump($bar);
+?>
+===DONE===
+<?php exit(0); ?>
+--EXPECTF--
+array(2) {
+  [0]=>
+  &object(stdClass)#1 (0) {
+  }
+  [1]=>
+  &object(stdClass)#1 (0) {
+  }
+}
+array(2) {
+  [0]=>
+  &object(stdClass)#1 (0) {
+  }
+  [1]=>
+  &object(stdClass)#1 (0) {
+  }
+}
+array(2) {
+  [0]=>
+  &object(stdClass)#2 (0) {
+  }
+  [1]=>
+  &object(stdClass)#2 (0) {
+  }
+}
+array(2) {
+  [0]=>
+  &string(3) "roh"
+  [1]=>
+  &string(3) "roh"
+}
+===DONE===