Race condition on the refcount of that array.

I had a bunch of SEGVs at high load, which is why the 
patch was inserted to check for these specific cases.

And the segvs stopped ... do you want me to take a second look ? :)

Thank you for your prompt reply.

Knowing that it's refcount related allowed me to reproduce the SEGV with my modified (needcopy=0) version of APC.  Here's the test script I ran many times concurrently:

<?php

function x($a = array(1, 2, 3, 4, 5))
{
}

for ($i = 0; $i < 100000; $i++)
{
    x();
}

?>

This crashed PHP 4.4.7, but not PHP 5.2.1.  (It also turns out that mmcache crashes on this - heh.)

This leaves two questions in my mind:

- Is it still an issue as of the most recent versions of PHP5?
- Is there a workaround that doesn't require copying the entire function into local process space?

As for it being fixed in php-5.x, I have completely forgotten
*where* the race condition was (*heh*) to reconfirm it is 
fixed now (somewhere in RECV_INIT opcode). I lost my 
valgrind patches when my main OS disk crashed, which is 
what I was using to track race conditions. They will 
have to be re-synthesised for the latest valgrind
before I can debug this.

The workaround typically involves fixing Zend if it still 
clobbers refcount in those arrays. Sort of one leading to 
the other.

I managed to crash PHP 5.2.3 with my non-copying APC.  The reason it didn't crash before was a matter of hardware.  My 4.4.7 test box was a dual-cpu hyperthreading xeon, and the 5.2.1 box was a single athlon cpu.  When I built and tested 5.2.3 on the dual-cpu box, the script crashed readily.

I took a closer look at what's going on.  As you mentioned already, it looks like a refcount race condition.  Because RECV_INIT only does a shallow copy of a CONSTANT_ARRAY parameter, all of the member zvals are still in shared memory.  The shallow copy (zval_copy_ctor) increments (++) the member zval refcounts.

Later, after the function returns, the caller does a zend_hash_clean on the old symbol table.  This frees the shallow-copied array, and decrements (--) the refcounts of the member zvals (still in shared memory).

Because the C ++ and -- operators are non-atomic (load, modify, store), and the Zend engine does no mutex locking of refcounts, _zval_ptr_dtor will eventually attempt to free the zval in shared memory.

I verified this with two different quick & dirty fixes.  The first fix was to use a SYSV semaphore to prevent RECV_INIT's zval_copy_ctor from running concurrently with the zend_hash_clean on the symbol table.  Although execution was slowed by an order of magnitude, the SEGV went away.  An alternate fix I tried involved adding an "immortal" flag to the zval struct.  I had APC set the flag on all CONSTANT_ARRAY members, and changed _zval_ptr_dtor to do nothing when the "immortal" flag is set.  This also made the SEGV go away.

Because the race condition is on the refcounts of the array members, the most obvious improvement would be to deep-copy only for non-empty arrays.  In my application, the empty array makes up the overwhelming majority of default-parameter cases, so this simple patch would be enough to satisfy my needs.

Since I haven't heard anything about this in over a month, I'll paste the patch I've been using to work around the memory issue.  The idea here is that an empty CONSTANT_ARRAY shouldn't force a deep copy, since the race condition is on array members (empty array = no members = no crash).

--- APC-3.0.14-orig/apc_compile.c       2007-04-02 19:05:30.000000000 -0400
+++ APC-3.0.14/apc_compile.c    2007-07-16 12:22:41.000000000 -0400
@@ -1235,7 +1235,8 @@
 #endif
             case ZEND_RECV_INIT:
                 if(zo->op2.op_type == IS_CONST &&
-                    zo->op2.u.constant.type == IS_CONSTANT_ARRAY) {
+                    zo->op2.u.constant.type == IS_CONSTANT_ARRAY &&
+                    zo->op2.u.constant.value.ht->nNumOfElements) {
                     if(flags != NULL) {
                         flags->deep_copy = 1;
                     }
@@ -1243,9 +1244,11 @@
                 break;
             default:
                 if((zo->op1.op_type == IS_CONST &&
-                    zo->op1.u.constant.type == IS_CONSTANT_ARRAY) ||
+                    zo->op1.u.constant.type == IS_CONSTANT_ARRAY &&
+                    zo->op1.u.constant.value.ht->nNumOfElements) ||
                     (zo->op2.op_type == IS_CONST &&
-                        zo->op2.u.constant.type == IS_CONSTANT_ARRAY)) {
+                        zo->op2.u.constant.type == IS_CONSTANT_ARRAY &&
+                        zo->op2.u.constant.value.ht->nNumOfElements)) {
                     if(flags != NULL) {
                         flags->deep_copy = 1;
                     }
@@ -2010,9 +2013,11 @@
         zo = &src->opcodes[j];

         if( ((zo->op1.op_type == IS_CONST &&
-              zo->op1.u.constant.type == IS_CONSTANT_ARRAY)) ||
+              zo->op1.u.constant.type == IS_CONSTANT_ARRAY) &&
+              zo->op1.u.constant.value.ht->nNumOfElements) ||
             ((zo->op2.op_type == IS_CONST &&
-              zo->op2.u.constant.type == IS_CONSTANT_ARRAY))) {
+              zo->op2.u.constant.type == IS_CONSTANT_ARRAY) &&
+              zo->op2.u.constant.value.ht->nNumOfElements)) {
             needcopy = 1;
         }
     }

As a follow up to this, I'm currently beginning tests of this in our production environment.  I'll try to follow up with any problems I find in the next few weeks. (it seems to be working fine for now).

This has been working pretty consistently for me in production the last few months, probably worth investigating more at some point to see if we can replicate this bug.

Closing this bug for several reasons:

 5.2 is unsupported software.
 APC is no longer the primary means of caching opcodes.

Thanks for helping to make PHP better :)