From cd442db9e1f359f5866d089dab22f2206e6d62a7 Mon Sep 17 00:00:00 2001 From: Tyson Andre Date: Sun, 24 Jul 2016 14:43:30 -0700 Subject: [PATCH 1/2] Add failing test of objects being cloned, not assigned. And a test of references, which were already working. --- tests/apc_005b.phpt | 51 +++++++++++++++++++++++++++++++++++++++ tests/apc_005c.phpt | 59 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+) create mode 100644 tests/apc_005b.phpt create mode 100644 tests/apc_005c.phpt diff --git a/tests/apc_005b.phpt b/tests/apc_005b.phpt new file mode 100644 index 00000000..197af1a6 --- /dev/null +++ b/tests/apc_005b.phpt @@ -0,0 +1,51 @@ +--TEST-- +APC: apcu_store/fetch with arrays with duplicate object +--SKIPIF-- + +--INI-- +apc.enabled=1 +apc.enable_cli=1 +apc.file_update_protection=0 +--FILE-- + +===DONE=== + +--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=== diff --git a/tests/apc_005c.phpt b/tests/apc_005c.phpt new file mode 100644 index 00000000..a8349bba --- /dev/null +++ b/tests/apc_005c.phpt @@ -0,0 +1,59 @@ +--TEST-- +APC: apcu_store/fetch with arrays with two object references +--SKIPIF-- + +--INI-- +apc.enabled=1 +apc.enable_cli=1 +apc.file_update_protection=0 +--FILE-- + +===DONE=== + +--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=== From 278aedb6da1e923330493fff03496f8401755dea Mon Sep 17 00:00:00 2001 From: Tyson Andre Date: Sun, 24 Jul 2016 13:44:24 -0700 Subject: [PATCH 2/2] Fix memory leak/failed check for duplicates when serializing arrays and object Affects built in serializer as well as the igbinary7 serializer. The apc serialization/unserialization code was checking IS_REFCOUNTED_P(val). However, when objects were being serialized as strings, the IS_REFCOUNTED bit in the type flags wasn't being set (Bitmask was 0) This meant that if there were two uses of an object being serialized, it would be cloned, since the code treated `dst` as if it couldn't be refcounted, and thus didn't add the mapping from `src`'s zend_refcounted pointer to `dst` into `si->copied` Found this bug while working on https://github.com/igbinary/igbinary7/pull/23 --- apc_cache.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/apc_cache.c b/apc_cache.c index e9cd3a1d..59f3cd43 100644 --- a/apc_cache.c +++ b/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,6 +1537,7 @@ 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; @@ -1541,8 +1545,7 @@ static APC_HOTSPOT zval* my_copy_zval(zval* dst, const zval* src, apc_context_t* 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,6 +1558,9 @@ 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: @@ -1562,6 +1568,7 @@ static APC_HOTSPOT zval* my_copy_zval(zval* dst, const zval* src, apc_context_t* 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);