Re: [apc] patch to fix class inherit duplicate method opcode bug

Could you clarify a bit what you are trying to fix? Is it this situation? file A defines class A file B has class B which extends A If you update a method in file A in class A then calling this method from an instantiate B object ends up calling the original method as it was prior to the update since all the methods from A is copied into B at compile time. I do have a pseudo-working patch for this problem, but I have been chasing a shared memory corruption bug and didn't want to complicate things yet. -Rasmus Xuefer wrote:

i can't find the bug reported by myself long ago, it's just missing so i post the patch here and my_uninsulate part of the patch is from eacelerator(turck-mmcache) with GPL :( but it's so simple, almost the same as install_function()/install_class() in apc. you might rewrite one for licence issue. the other part is written by myself. the do/while/if part is a rewritten code just for better flow, with lower chance to get mistake. and i also recommend this struct to other part of source code in apc. maybe i can be better. ------------------------------------------------------------------------ Index: apc_main.c =================================================================== RCS file: /repository/pecl/apc/apc_main.c,v retrieving revision 3.27 diff -u -r3.27 apc_main.c --- apc_main.c 3 Feb 2005 15:33:12 -0000 3.27 +++ apc_main.c 14 Feb 2005 16:56:39 -0000 @@ -119,6 +117,102 @@ } /* }}} */ +/* {{{ my_insulate + Overrides zend_compile_file */ +typedef struct {

+    HashTable* orig_function_table;
+    HashTable* orig_class_table;
+    HashTable tmp_function_table;
+    HashTable tmp_class_table;

+} my_insulate_storage; + +static void my_insulate(my_insulate_storage *p TSRMLS_DC) +{

+    zend_hash_init_ex(&(p->tmp_function_table), 100, NULL, NULL, 1, 0);
+    p->orig_function_table = CG(function_table);
+    CG(function_table) = &(p->tmp_function_table);

+

+    zend_hash_init_ex(&(p->tmp_class_table), 10, NULL, NULL, 1, 0);
+    p->orig_class_table = CG(class_table);
+    CG(class_table) = &(p->tmp_class_table);

+} + +static void my_uninsulate(my_insulate_storage *p, zend_file_handle* file_handle TSRMLS_DC) +{

+    Bucket *p_function;
+    Bucket *p_class;

+

+    p_function = p->tmp_function_table.pListHead;
+    p_class = p->tmp_class_table.pListHead;

+

+    CG(function_table) = p->orig_function_table;
+    CG(class_table) = p->orig_class_table;

+

+    /* install function */
+    while (p_function != NULL) {
+        zend_op_array *op_array = (zend_op_array*)p_function->pData;
+        if (op_array->type == ZEND_USER_FUNCTION) {
+            if (zend_hash_add(CG(function_table),
+                        p_function->arKey,
+                        p_function->nKeyLength,
+                        op_array, sizeof(zend_op_array), NULL) == FAILURE &&
+                    p_function->arKey[0] != '\000') {
+                CG(in_compilation) = 1;
+                CG(compiled_filename) = file_handle->opened_path;

+#ifdef ZEND_ENGINE_2

+                CG(zend_lineno) = op_array->line_start;

+#else

+                CG(zend_lineno) = op_array->opcodes[0].lineno;

+#endif

+                zend_error(E_ERROR, "Cannot redeclare %s()", p_function->arKey);
+            }
+        }
+        p_function = p_function->pListNext;
+    }

+

+    /* install class */
+    while (p_class != NULL) {

+#ifdef ZEND_ENGINE_2

+        zend_class_entry **ce = (zend_class_entry**)p_class->pData;
+        if ((*ce)->type == ZEND_USER_CLASS) {
+            if (zend_hash_add(CG(class_table),
+                        p_class->arKey,
+                        p_class->nKeyLength,
+                        ce, sizeof(zend_class_entry*), NULL) == FAILURE &&
+                    p_class->arKey[0] != '\000') {
+                CG(in_compilation) = 1;
+                CG(compiled_filename) = file_handle->opened_path;
+                CG(zend_lineno) = (*ce)->line_start;
+                zend_error(E_ERROR, "Cannot redeclare class %s", p_class->arKey);
+            }
+        }

+#else

+        zend_class_entry *ce = (zend_class_entry*)p_class->pData;
+        if (ce->type == ZEND_USER_CLASS) {
+            if (ce->parent != NULL) {
+                if (zend_hash_find(CG(class_table), (void*)ce->parent->name, ce->parent->name_length+1, (void **)&ce->parent) != SUCCESS) {
+                    ce->parent = NULL;
+                }
+            }
+            if (zend_hash_add(CG(class_table),
+                        p_class->arKey,
+                        p_class->nKeyLength,
+                        ce, sizeof(zend_class_entry), NULL) == FAILURE &&
+                    p_class->arKey[0] != '\000') {
+                CG(in_compilation) = 1;
+                CG(compiled_filename) = file_handle->opened_path;
+                CG(zend_lineno) = 0;
+                zend_error(E_ERROR, "Cannot redeclare class %s", p_class->arKey);
+            }
+        }

+#endif

+        p_class = p_class->pListNext;
+    }
+    zend_hash_destroy(&(p->tmp_function_table));
+    zend_hash_destroy(&(p->tmp_class_table));

+} +/* }}} */ + /* {{{ my_compile_file

    Overrides zend_compile_file */

static zend_op_array* my_compile_file(zend_file_handle* h, @@ -132,10 +226,16 @@

     apc_function_t* alloc_functions;
     apc_class_t* alloc_classes;
     time_t t;
+    my_insulate_storage my_ins;
+    int bailout;

+

+    if (!h->opened_path) {
+        return old_compile_file(h, type TSRMLS_CC);
+    }
      /* check our regular expression filters first */
     if (APCG(compiled_filters)) {
-        int ret = apc_regex_match_array(APCG(compiled_filters), h->filename);
+        int ret = apc_regex_match_array(APCG(compiled_filters), h->opened_path);
         if(ret == APC_NEGATIVE_MATCH || (ret != APC_POSITIVE_MATCH && !APCG(cache_by_default))) {
             return old_compile_file(h, type TSRMLS_CC);
         }

@@ -152,12 +252,12 @@ #endif

     /* try to create a cache key; if we fail, give up on caching */
-    if (!apc_cache_make_file_key(&key, h->filename, PG(include_path), t TSRMLS_CC)) {
+    if (!apc_cache_make_file_key(&key, h->opened_path, PG(include_path), t TSRMLS_CC)) {
         return old_compile_file(h, type TSRMLS_CC);
     }
          /* search for the file in the cache */
-    cache_entry = apc_cache_find(APCG(cache), key, t);
+    cache_entry = apc_cache_find(apc_cache, key, t);
     if (cache_entry != NULL) {
         int dummy = 1;
         if (h->opened_path == NULL) {

@@ -166,62 +266,77 @@

         zend_hash_add(&EG(included_files), h->opened_path, strlen(h->opened_path)+1, (void *)&dummy, sizeof(int), NULL);
         zend_llist_add_element(&CG(open_files), h); /* XXX kludge */
         apc_stack_push(APCG(cache_stack), cache_entry);
-        return cached_compile(TSRMLS_C);
+        return cached_compile(cache_entry TSRMLS_CC);
     }
 +    /* move to a new func/class env */
+    my_insulate(&my_ins TSRMLS_CC);

+

     /* remember how many functions and classes existed before compilation */
     num_functions = zend_hash_num_elements(CG(function_table));
     num_classes   = zend_hash_num_elements(CG(class_table));
     -    /* compile the file using the default compile function */
-    op_array = old_compile_file(h, type TSRMLS_CC);
-    if (op_array == NULL) {
-        return NULL;
+    zend_try {
+        bailout = 0;
+        /* compile the file using the default compile function */
+        op_array = old_compile_file(h, type TSRMLS_CC);
+        if (op_array == NULL) {
+            return NULL;
+        }
+    } zend_catch {
+        bailout = 1;
+    } zend_end_try();
+    if (bailout) {
+        my_uninsulate(&my_ins, h TSRMLS_CC);
+        zend_bailout();
     }
      HANDLE_BLOCK_INTERRUPTIONS();
-    if(!(alloc_op_array = apc_copy_op_array(NULL, op_array, apc_sma_malloc, apc_sma_free TSRMLS_CC))) {
-        apc_cache_expunge(APCG(cache),t);
-        HANDLE_UNBLOCK_INTERRUPTIONS();
-        apc_log(APC_WARNING, "(apc_copy_op_array) unable to cache '%s': insufficient " "shared memory available", h->opened_path?h->opened_path:"");
-        return op_array;
-    }
-    -    if(!(alloc_functions = apc_copy_new_functions(num_functions, apc_sma_malloc, apc_sma_free TSRMLS_CC))) {
-        apc_free_op_array(alloc_op_array, apc_sma_free);
-        apc_cache_expunge(APCG(cache),t);
-        HANDLE_UNBLOCK_INTERRUPTIONS();
-        apc_log(APC_WARNING, "(apc_copy_new_functions) unable to cache '%s': insufficient " "shared memory available", h->opened_path?h->opened_path:"");
-        return op_array;
-    }
-    if(!(alloc_classes = apc_copy_new_classes(op_array, num_classes, apc_sma_malloc, apc_sma_free TSRMLS_CC))) {
-        apc_free_op_array(alloc_op_array, apc_sma_free);
-        apc_free_functions(alloc_functions, apc_sma_free);
-        apc_cache_expunge(APCG(cache),t);
-        HANDLE_UNBLOCK_INTERRUPTIONS();
-        apc_log(APC_WARNING, "(apc_copy_new_classes) unable to cache '%s': insufficient " "shared memory available", h->opened_path?h->opened_path:"");
-        return op_array;
-    }

-

-    if(!(cache_entry = apc_cache_make_file_entry(h->opened_path, alloc_op_array, alloc_functions, alloc_classes))) {
-        apc_free_op_array(alloc_op_array, apc_sma_free);
-        apc_free_functions(alloc_functions, apc_sma_free);
-        apc_free_classes(alloc_classes, apc_sma_free);
-        apc_cache_expunge(APCG(cache),t);
+    do {
+        alloc_classes = NULL;
+        alloc_functions = NULL;
+        alloc_op_array = NULL;
+        if(!(alloc_op_array = apc_copy_op_array(NULL, op_array, apc_sma_malloc, apc_sma_free TSRMLS_CC))) {
+            apc_log(APC_WARNING, "(apc_copy_op_array) unable to cache '%s': insufficient " "shared memory available", h->opened_path);
+            break;
+        }
+        if(!(alloc_functions = apc_copy_new_functions(num_functions, apc_sma_malloc, apc_sma_free TSRMLS_CC))) {
+            apc_log(APC_WARNING, "(apc_copy_new_functions) unable to cache '%s': insufficient " "shared memory available", h->opened_path);
+            break;
+        }
+        if(!(alloc_classes = apc_copy_new_classes(op_array, num_classes, apc_sma_malloc, apc_sma_free TSRMLS_CC))) {
+            apc_log(APC_WARNING, "(apc_copy_new_classes) unable to cache '%s': insufficient " "shared memory available", h->opened_path);
+            break;
+        }
+        if(!(cache_entry = apc_cache_make_file_entry(h->opened_path, alloc_op_array, alloc_functions, alloc_classes))) {
+            apc_log(APC_WARNING, "(apc_cache_make_file_entry) unable to cache '%s': insufficient " "shared memory available", h->opened_path);
+            break;
+        }
+    } while (0);
+    if (cache_entry) {
         HANDLE_UNBLOCK_INTERRUPTIONS();
-        apc_log(APC_WARNING, "(apc_cache_make_file_entry) unable to cache '%s': insufficient " "shared memory available", h->opened_path?h->opened_path:"");
-        return op_array;
+        if ((ret = apc_cache_insert(apc_cache, key, cache_entry, t)) != 1) {
+            apc_cache_free_entry(cache_entry);
+            if (ret==-1) {
+                apc_cache_expunge(apc_cache,t);
+                apc_log(APC_WARNING, "(apc_cache_insert) unable to cache '%s': insufficient " "shared memory available", h->opened_path);
+            }
+        }
     }
-    HANDLE_UNBLOCK_INTERRUPTIONS();

-

-    if ((ret = apc_cache_insert(APCG(cache), key, cache_entry, t)) != 1) {
-        apc_cache_free_entry(cache_entry);
-        if(ret==-1) {
-            apc_cache_expunge(APCG(cache),t);
-            apc_log(APC_WARNING, "(apc_cache_insert) unable to cache '%s': insufficient " "shared memory available", h->opened_path?h->opened_path:"");
+    else {
+        if (alloc_classes != NULL) {
+            apc_free_classes(alloc_classes, apc_sma_free);
+        }
+        if (alloc_functions != NULL) {
+            apc_free_functions(alloc_functions, apc_sma_free);
         }
+        if (alloc_op_array != NULL) {
+            apc_free_op_array(alloc_op_array, apc_sma_free);
+        }
+        apc_cache_expunge(apc_cache,t);
+        HANDLE_UNBLOCK_INTERRUPTIONS();
     }

-

+    my_uninsulate(&my_ins, h TSRMLS_CC);
     return op_array;

} /* }}} */