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
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: pr at bgagroup dot net
New email:
PHP Version: OS:

 

 [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

Pull Requests

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-2025 The PHP Group
All rights reserved.
Last updated: Sat Jul 12 13:01:33 2025 UTC