php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #58274 apc_store/apc_fetch do not handle reference count / references properly
Submitted: 2008-07-15 11:21 UTC Modified: 2008-07-17 12:41 UTC
From: pr at bgagroup dot net Assigned:
Status: Closed Package: APC (PECL)
PHP Version: 5.2.4 OS: Linux
Private report: No CVE-ID: None
 [2008-07-15 11:21 UTC] pr at bgagroup dot net
Description:
------------
apc_store simply copies the complete zval of deep data structures.
so after getting the data with apc_fetch, it will still reflect the original reference count, which prohibits proper garbage collection.

Reproduce code:
---------------
$a = 'a';
$b = array($a);
$c = array('c');
$b[] = &$c;
$b[] = &$c;
$d = 'd';
$b[] = &$d;
$b[] = &$d;
$b[] = &$d;
apc_store('test', $b);
$x = apc_fetch('test');
debug_zval_dump($x);

Expected result:
----------------
array(6) refcount(2){
  [0]=>
  string(1) "a" refcount(1)
  [1]=>
  &array(1) refcount(2){
    [0]=>
    string(1) "c" refcount(1)
  }
  [2]=>
  &array(1) refcount(2){
    [0]=>
    string(1) "c" refcount(1)
  }
  [3]=>
  &string(1) "d" refcount(3)
  [4]=>
  &string(1) "d" refcount(3)
  [5]=>
  &string(1) "d" refcount(3)
}

Actual result:
--------------
array(6) refcount(2){
  [0]=>
  string(1) "a" refcount(2)
  [1]=>
  &array(1) refcount(3){
    [0]=>
    string(1) "c" refcount(1)
  }
  [2]=>
  &array(1) refcount(3){
    [0]=>
    string(1) "c" refcount(1)
  }
  [3]=>
  &string(1) "d" refcount(4)
  [4]=>
  &string(1) "d" refcount(4)
  [5]=>
  &string(1) "d" refcount(4)
}

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2008-07-15 11:30 UTC] pr at bgagroup dot net
the following patch fixes the test case but might break everything else.

--- apc_compile.c.orig	2008-07-15 17:23:55.000000000 +0200
+++ apc_compile.c	2008-07-15 17:27:50.000000000 +0200
@@ -316,9 +316,6 @@
         *dst = dst_new;
     }
 
-    Z_SET_REFCOUNT_PP(dst, Z_REFCOUNT_PP((zval**)src));
-    Z_SET_ISREF_TO_PP(dst, Z_ISREF_PP((zval**)src));
-    
     return dst;
 }
 /* }}} */
@@ -333,7 +330,16 @@
     assert(src != NULL);
 
     memcpy(dst, src, sizeof(src[0]));
-
+	INIT_PZVAL(dst);
+    if(APCG(copied_zvals)) {
+        if(zend_hash_index_find(APCG(copied_zvals), (ulong)src, (void**)&tmp) == SUCCESS) {
+            Z_SET_ISREF_PP(tmp);
+            Z_ADDREF_PP(tmp);
+            return *tmp;
+        }
+        zend_hash_index_update(APCG(copied_zvals), (ulong)src, (void**)&dst, sizeof(zval*), NULL);
+    }
+ 
     switch (src->type & ~IS_CONSTANT_INDEX) {
     case IS_RESOURCE:
     case IS_BOOL:
@@ -356,14 +362,6 @@
     
     case IS_ARRAY:
 
-        if(APCG(copied_zvals)) {
-            if(zend_hash_index_find(APCG(copied_zvals), (ulong)src, (void**)&tmp) == SUCCESS) {
-                Z_ADDREF_PP(tmp);
-                return *tmp;
-            }
-        
-            zend_hash_index_update(APCG(copied_zvals), (ulong)src, (void**)&dst, sizeof(zval*), NULL);
-        }
         /* fall through */
  
     case IS_CONSTANT_ARRAY:
@@ -1635,6 +1633,15 @@
     zval **tmp;
     TSRMLS_FETCH();
 
+    /* Maintain a list of zvals we've copied to properly handle recursive structures */
+    if(APCG(copied_zvals)) {
+        if(zend_hash_index_find(APCG(copied_zvals), (ulong)src, (void**)&tmp) == SUCCESS) {
+            Z_DELREF_PP(tmp);
+            return FAILURE;
+        } 
+        zend_hash_index_update(APCG(copied_zvals), (ulong)src, (void**)&src, sizeof(zval*), NULL);
+    }
+    
     switch (src->type & ~IS_CONSTANT_INDEX) {
     case IS_RESOURCE:
     case IS_BOOL:
@@ -1653,14 +1660,6 @@
     
     case IS_ARRAY:
     
-        /* Maintain a list of zvals we've copied to properly handle recursive structures */
-        if(APCG(copied_zvals)) {
-            if(zend_hash_index_find(APCG(copied_zvals), (ulong)src, (void**)&tmp) == SUCCESS) {
-                Z_DELREF_PP(tmp);
-                return FAILURE;
-            } 
-            zend_hash_index_update(APCG(copied_zvals), (ulong)src, (void**)&src, sizeof(zval*), NULL);
-        }
         /* fall through */
 
     case IS_CONSTANT_ARRAY:
 [2008-07-16 05:17 UTC] pr at bgagroup dot net
Improved patch against HEAD and test case
- copy references as references
- refcount needs to be initialized before recursion
- refcount must only be touched for apc_fetch/store code path

--- apc_compile.c	16 Jul 2008 06:30:55 -0000	3.107
+++ apc_compile.c	16 Jul 2008 09:08:50 -0000
@@ -286,7 +286,13 @@
     memcpy(dst, src, sizeof(src[0]));
 
     if(APCG(copied_zvals)) {
+        /* deep copies are refcount(1) */
+        Z_SET_REFCOUNT_P(dst, 1);
+        Z_UNSET_ISREF_P(dst);
         if(zend_hash_index_find(APCG(copied_zvals), (ulong)src, (void**)&tmp) == SUCCESS) {
+        	if(Z_ISREF_P(src)) {
+        		Z_SET_ISREF_PP(tmp);
+        	}
             Z_ADDREF_PP(tmp);
             return *tmp;
         }
@@ -336,10 +342,6 @@
         assert(0);
     }
 
-    /* deep copies are refcount(1) */
-    Z_SET_REFCOUNT_P(dst, 1);
-    Z_UNSET_ISREF_P(dst);
-
     return dst;
 }
 /* }}} */


--TEST--
APC: apc_store/fetch reference test
--INI--
apc.enabled=1
apc.enable_cli=1
apc.file_update_protection=0
--FILE--
<?php

$a = 'a';
$b = array($a);
$c = array('c');
$b[] = &$c;
$b[] = &$c;
$d = 'd';
$b[] = &$d;
$b[] = &$d;
$b[] = &$d;
$e = 'e';
$b[] = $e;
$b[] = $e;
$f = array('f');
$f[] = &$f;
$b[] = &$f;
apc_store('test', $b);
$x = apc_fetch('test');
debug_zval_dump($x);

?>
===DONE===
<?php exit(0); ?>
--EXPECTF--
array(9) refcount(2){
  [0]=>
  string(1) "a" refcount(1)
  [1]=>
  &array(1) refcount(2){
    [0]=>
    string(1) "c" refcount(1)
  }
  [2]=>
  &array(1) refcount(2){
    [0]=>
    string(1) "c" refcount(1)
  }
  [3]=>
  &string(1) "d" refcount(3)
  [4]=>
  &string(1) "d" refcount(3)
  [5]=>
  &string(1) "d" refcount(3)
  [6]=>
  string(1) "e" refcount(2)
  [7]=>
  string(1) "e" refcount(2)
  [8]=>
  &array(2) refcount(2){
    [0]=>
    string(1) "f" refcount(1)
    [1]=>
    &array(2) refcount(2){
      [0]=>
      string(1) "f" refcount(1)
      [1]=>
      *RECURSION*
    }
  }
}
===DONE===
 [2008-07-17 12:41 UTC] gopalv82 at yahoo dot com
Whoa, my bad ... missed that bit about refcounts being reset

In my "fix" my_copy_zval(&f) does deep, calls my_copy_zval(&f) which does *tmp shallow ... but then the deep goes and sets its refcount to 1.

Fixed that bit now & thanks for the test case (it passeth now)
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu May 02 06:01:32 2024 UTC