fix: safe opcache_reset

caddy/caddy_test.go

@@ -3,6 +3,7 @@ package caddy_test
 import (
 	"bytes"
 	"fmt"
+	"math/rand/v2"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -1541,6 +1542,72 @@ func TestDd(t *testing.T) {
 	)
 }
 
+// test to force the opcache segfault race condition under concurrency (~1.7s)
+func TestOpcacheReset(t *testing.T) {
+	tester := caddytest.NewTester(t)
+	tester.InitServer(`
+		{
+			skip_install_trust
+			admin localhost:2999
+			http_port `+testPort+`
+			metrics
+
+			frankenphp {
+				num_threads 40
+				php_ini {
+					opcache.enable 1
+					opcache.log_verbosity_level 4
+				}
+			}
+		}
+
+		localhost:`+testPort+` {
+			php {
+				root ../testdata
+				worker {
+                    file sleep.php
+                    match /sleep*
+                    num 20
+                }
+			}
+		}
+		`, "caddyfile")
+
+	wg := sync.WaitGroup{}
+	numRequests := 100
+	wg.Add(numRequests)
+	for i := 0; i < numRequests; i++ {
+
+		// introduce some random delay
+		if rand.IntN(10) > 8 {
+			time.Sleep(time.Millisecond * 10)
+		}
+
+		go func() {
+			// randomly call opcache_reset
+			if rand.IntN(10) > 5 {
+				tester.AssertGetResponse(
+					"http://localhost:"+testPort+"/opcache_reset.php",
+					http.StatusOK,
+					"opcache reset done",
+				)
+				wg.Done()
+				return
+			}
+
+			// otherwise call sleep.php with random sleep and work values
+			tester.AssertGetResponse(
+				fmt.Sprintf("http://localhost:%s/sleep.php?sleep=%d&work=%d", testPort, i, i),
+				http.StatusOK,
+				fmt.Sprintf("slept for %d ms and worked for %d iterations", i, i),
+			)
+			wg.Done()
+		}()
+	}
+
+	wg.Wait()
+}
+
 func TestLog(t *testing.T) {
 	tester := caddytest.NewTester(t)
 	initServer(t, tester, `

frankenphp.c

@@ -87,6 +87,26 @@ __thread uintptr_t thread_index;
 __thread bool is_worker_thread = false;
 __thread HashTable *sandboxed_env = NULL;
 
+/* Forward declaration */
+PHP_FUNCTION(frankenphp_opcache_reset);
+zif_handler orig_opcache_reset;
+
+/* Try to override opcache_reset if opcache is loaded.
+ * Safe to call multiple times - skips if already overridden in this function
+ * table. Uses handler comparison instead of orig_opcache_reset check so that
+ * a fresh function table after PHP module restart is always re-overridden. */
+static void frankenphp_override_opcache_reset(void) {
+  zend_function *func = zend_hash_str_find_ptr(
+      CG(function_table), "opcache_reset", sizeof("opcache_reset") - 1);
+  if (func != NULL && func->type == ZEND_INTERNAL_FUNCTION &&
+      ((zend_internal_function *)func)->handler !=
+          ZEND_FN(frankenphp_opcache_reset)) {
+    orig_opcache_reset = ((zend_internal_function *)func)->handler;
+    ((zend_internal_function *)func)->handler =
+        ZEND_FN(frankenphp_opcache_reset);
+  }
+}
+
 void frankenphp_update_local_thread_context(bool is_worker) {
   is_worker_thread = is_worker;
 
@@ -457,6 +477,13 @@ PHP_FUNCTION(frankenphp_getenv) {
   }
 } /* }}} */
 
+/* {{{ thread-safe opcache reset */
+PHP_FUNCTION(frankenphp_opcache_reset) {
+  go_schedule_opcache_reset(thread_index);
+
+  RETVAL_TRUE;
+} /* }}} */
+
 /* {{{ Fetch all HTTP request headers */
 PHP_FUNCTION(frankenphp_request_headers) {
   ZEND_PARSE_PARAMETERS_NONE();
@@ -715,6 +742,10 @@ PHP_MINIT_FUNCTION(frankenphp) {
     php_error(E_WARNING, "Failed to find built-in getenv function");
   }
 
+  // Override opcache_reset (may not be available yet if opcache loads as a
+  // shared extension in PHP 8.4 and below)
+  frankenphp_override_opcache_reset();
+
   return SUCCESS;
 }
 
@@ -733,7 +764,16 @@ static zend_module_entry frankenphp_module = {
 static int frankenphp_startup(sapi_module_struct *sapi_module) {
   php_import_environment_variables = get_full_env;
 
-  return php_module_startup(sapi_module, &frankenphp_module);
+  int result = php_module_startup(sapi_module, &frankenphp_module);
+#if PHP_VERSION_ID < 80500
+  if (result == SUCCESS) {
+    /* Override opcache here again if loaded as a shared extension
+     * (php 8.4 and under) */
+    frankenphp_override_opcache_reset();
+  }
+#endif
+
+  return result;
 }
 
 static int frankenphp_deactivate(void) { return SUCCESS; }
@@ -1195,6 +1235,11 @@ bool frankenphp_new_php_thread(uintptr_t thread_index) {
 static int frankenphp_request_startup() {
   frankenphp_update_request_context();
   if (php_request_startup() == SUCCESS) {
+#if PHP_VERSION_ID < 80500
+    /* Override opcache here again if loaded as a shared extension
+     * (php 8.4 and under) */
+    frankenphp_override_opcache_reset();
+#endif
     return SUCCESS;
   }
 
@@ -1394,12 +1439,12 @@ int frankenphp_execute_script_cli(char *script, int argc, char **argv,
 }
 
 int frankenphp_reset_opcache(void) {
-  zend_function *opcache_reset =
-      zend_hash_str_find_ptr(CG(function_table), ZEND_STRL("opcache_reset"));
-  if (opcache_reset) {
-    zend_call_known_function(opcache_reset, NULL, NULL, NULL, 0, NULL, NULL);
-  }
-
+  zend_execute_data execute_data;
+  zval retval;
+  memset(&execute_data, 0, sizeof(execute_data));
+  ZVAL_UNDEF(&retval);
+  orig_opcache_reset(&execute_data, &retval);
+  zval_ptr_dtor(&retval);
   return 0;
 }
 

frankenphp.go

@@ -32,11 +32,14 @@ import (
 	"runtime"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"syscall"
 	"time"
 	"unsafe"
 	// debug on Linux
 	//_ "github.com/ianlancetaylor/cgosymbolizer"
+
+	"github.com/dunglas/frankenphp/internal/state"
 )
 
 type contextKeyStruct struct{}
@@ -56,8 +59,9 @@ var (
 	contextKey   = contextKeyStruct{}
 	serverHeader = []string{"FrankenPHP"}
 
-	isRunning        bool
-	onServerShutdown []func()
+	isRunning            bool
+	threadsAreRestarting atomic.Bool
+	onServerShutdown     []func()
 
 	// Set default values to make Shutdown() idempotent
 	globalMu     sync.Mutex
@@ -754,6 +758,108 @@ func go_is_context_done(threadIndex C.uintptr_t) C.bool {
 	return C.bool(phpThreads[threadIndex].frankenPHPContext().isDone)
 }
 
+//export go_schedule_opcache_reset
+func go_schedule_opcache_reset(threadIndex C.uintptr_t) {
+	if threadsAreRestarting.CompareAndSwap(false, true) {
+		go restartThreadsAndOpcacheReset(true)
+	}
+}
+
+// opcacheResetOnce ensures only one thread calls the actual opcache_reset.
+// Multiple threads calling it concurrently can race on shared memory.
+var opcacheResetOnce sync.Once
+
+// restart all threads for an opcache_reset
+func restartThreadsAndOpcacheReset(withRegularThreads bool) {
+	// disallow scaling threads while restarting workers
+	scalingMu.Lock()
+	defer scalingMu.Unlock()
+
+	threadsToRestart := drainThreads(withRegularThreads)
+
+	opcacheResetOnce = sync.Once{}
+	opcacheResetWg := sync.WaitGroup{}
+	for _, thread := range threadsToRestart {
+		thread.state.Set(state.OpcacheResetting)
+		opcacheResetWg.Go(func() {
+			thread.state.WaitFor(state.OpcacheResettingDone)
+		})
+	}
+	opcacheResetWg.Wait()
+
+	for _, thread := range threadsToRestart {
+		thread.drainChan = make(chan struct{})
+		thread.state.Set(state.Ready)
+	}
+
+	threadsAreRestarting.Store(false)
+}
+
+func drainThreads(withRegularThreads bool) []*phpThread {
+	var (
+		ready          sync.WaitGroup
+		drainedThreads []*phpThread
+	)
+
+	for _, worker := range workers {
+		worker.threadMutex.RLock()
+		ready.Add(len(worker.threads))
+
+		for _, thread := range worker.threads {
+			if !thread.state.RequestSafeStateChange(state.Restarting) {
+				ready.Done()
+
+				// no state change allowed == thread is shutting down
+				// we'll proceed to restart all other threads anyway
+				continue
+			}
+			close(thread.drainChan)
+			drainedThreads = append(drainedThreads, thread)
+
+			go func(thread *phpThread) {
+				thread.state.WaitFor(state.Yielding)
+				ready.Done()
+			}(thread)
+		}
+
+		worker.threadMutex.RUnlock()
+	}
+
+	if withRegularThreads {
+		regularThreadMu.RLock()
+		ready.Add(len(regularThreads))
+
+		for _, thread := range regularThreads {
+			if !thread.state.RequestSafeStateChange(state.Restarting) {
+				ready.Done()
+
+				// no state change allowed == thread is shutting down
+				// we'll proceed to restart all other threads anyway
+				continue
+			}
+			close(thread.drainChan)
+			drainedThreads = append(drainedThreads, thread)
+
+			go func(thread *phpThread) {
+				thread.state.WaitFor(state.Yielding)
+				ready.Done()
+			}(thread)
+		}
+
+		regularThreadMu.RUnlock()
+	}
+
+	ready.Wait()
+
+	return drainedThreads
+}
+
+func scheduleOpcacheReset(thread *phpThread) {
+	opcacheResetOnce.Do(func() {
+		C.frankenphp_reset_opcache()
+	})
+}
+
 func convertArgs(args []string) (C.int, []*C.char) {
 	argc := C.int(len(args))
 	argv := make([]*C.char, argc)

internal/state/state.go

@@ -25,6 +25,8 @@ const (
 	// States necessary for restarting workers
 	Restarting
 	Yielding
+	OpcacheResetting
+	OpcacheResettingDone
 
 	// States necessary for transitioning between different handlers
 	TransitionRequested
@@ -52,6 +54,10 @@ func (s State) String() string {
 		return "restarting"
 	case Yielding:
 		return "yielding"
+	case OpcacheResetting:
+		return "opcache resetting"
+	case OpcacheResettingDone:
+		return "opcache reset done"
 	case TransitionRequested:
 		return "transition requested"
 	case TransitionInProgress:

testdata/opcache_reset.php

@@ -0,0 +1,9 @@
+<?php
+
+require_once __DIR__.'/_executor.php';
+
+return function () {
+    require __DIR__ .'/require.php';
+    opcache_reset();
+    echo "opcache reset done";
+};

testdata/require.php

@@ -0,0 +1,6 @@
+<?php
+
+// dummy require file for opcache_reset test
+return function (){
+    echo "";
+};

threadregular.go

@@ -1,5 +1,7 @@
 package frankenphp
 
+// #include "frankenphp.h"
+import "C"
 import (
 	"context"
 	"runtime"
@@ -46,7 +48,13 @@ func (handler *regularThread) beforeScriptExecution() string {
 		handler.state.Set(state.Ready)
 
 		return handler.waitForRequest()
-
+	case state.Restarting:
+		handler.state.Set(state.Yielding)
+		handler.state.WaitFor(state.OpcacheResetting)
+		scheduleOpcacheReset(handler.thread)
+		handler.state.Set(state.OpcacheResettingDone)
+		handler.state.WaitFor(state.Ready, state.ShuttingDown)
+		return handler.beforeScriptExecution()
 	case state.Ready:
 		return handler.waitForRequest()