fix: superglobals-realated crash with custom extensions in worker mode (#796)

* test: failing test reproducing #767

* fix

* Update frankenphp.c

Co-authored-by: Tim Düsterhus <timwolla@googlemail.com>

* Update frankenphp.c

Co-authored-by: Tim Düsterhus <timwolla@googlemail.com>

* review

* ZVAL_COPY

* fix

* add back current $_SERVER behavior

* add docs

* bad fix for the leak

* clean test

* improve tests

* fix test

* fix

* cleanup

* clarify destroy super globals name

* micro-optim: use zval_ptr_dtor_nogc to destroy super globals

* style

* fix

* better name for frankenphp_free_server_context

* more cleanup

* remove useless memset

* more cleanup

* continue refactoring

* fix and update docs

* docs

---------

Co-authored-by: Tim Düsterhus <timwolla@googlemail.com>

dev.Dockerfile

@@ -65,7 +65,7 @@ WORKDIR /go/src/app
 COPY . .
 
 WORKDIR /go/src/app/caddy/frankenphp
-RUN go build
+RUN go build -buildvcs=false
 
 WORKDIR /go/src/app
 CMD [ "zsh" ]

docs/cn/worker.md

@@ -68,11 +68,11 @@ $myApp->boot();
 
 // 循环外的处理程序以获得更好的性能（减少工作量）
 $handler = static function () use ($myApp) {
-        // 收到请求时调用
-        // 超全局变量 php://input
-        echo $myApp->handle($_GET, $_POST, $_COOKIE, $_FILES, $_SERVER);
+    // 收到请求时调用
+    // 超全局变量 php://input
+    echo $myApp->handle($_GET, $_POST, $_COOKIE, $_FILES, $_SERVER);
 };
-for($nbRequests = 0, $running = true; isset($_SERVER['MAX_REQUESTS']) && ($nbRequests < ((int)$_SERVER['MAX_REQUESTS'])) && $running; ++$nbRequests) {
+for ($nbRequests = 0, $running = true; isset($_SERVER['MAX_REQUESTS']) && ($nbRequests < ((int)$_SERVER['MAX_REQUESTS'])) && $running; ++$nbRequests) {
     $running = \frankenphp_handle_request($handler);
 
     // 发送 HTTP 响应后执行某些操作

docs/fr/worker.md

@@ -71,12 +71,12 @@ $myApp->boot();
 
 // En dehors de la boucle pour de meilleures performances (moins de travail effectué)
 $handler = static function () use ($myApp) {
-        // Appelé lorsqu'une requête est reçue,
-        // les superglobales, php://input, etc., sont réinitialisés
-        echo $myApp->handle($_GET, $_POST, $_COOKIE, $_FILES, $_SERVER);
+    // Appelé lorsqu'une requête est reçue,
+    // les superglobales, php://input, etc., sont réinitialisés
+    echo $myApp->handle($_GET, $_POST, $_COOKIE, $_FILES, $_SERVER);
 };
 
-for($nbRequests = 0, $running = true; isset($_SERVER['MAX_REQUESTS']) && ($nbRequests < ((int)$_SERVER['MAX_REQUESTS'])) && $running; ++$nbRequests) {
+for ($nbRequests = 0, $running = true; isset($_SERVER['MAX_REQUESTS']) && ($nbRequests < ((int)$_SERVER['MAX_REQUESTS'])) && $running; ++$nbRequests) {
     $running = \frankenphp_handle_request($handler);
 
     // Faire quelque chose après l'envoi de la réponse HTTP
@@ -117,3 +117,26 @@ Comme PHP n'a pas été initialement conçu pour des processus de longue durée,
 Une solution pour utiliser ce type de code en mode worker est de redémarrer le script worker après avoir traité un certain nombre de requêtes :
 
 Le code du worker précédent permet de configurer un nombre maximal de requêtes à traiter en définissant une variable d'environnement nommée `MAX_REQUESTS`.
+
+## Comportement des superglobales
+
+[Les superglobales PHP](https://www.php.net/manual/fr/language.variables.superglobals.php) (`$_SERVER`, `$_ENV`, `$_GET`...)
+se comportent comme suit :
+
+* avant le premier appel à `frankenphp_handle_request()`, les superglobales contiennent des valeurs liées au script worker lui-même
+* pendant et après l'appel à `frankenphp_handle_request()`, les superglobales contiennent des valeurs générées à partir de la requête HTTP traitée, chaque appel à `frankenphp_handle_request()` change les valeurs des superglobales
+
+Pour accéder aux superglobales du script worker à l'intérieur de la fonction de rappel, vous devez les copier et importer la copie dans le scope de la fonction :
+
+```php
+<?php
+// Copier la superglobale $_SERVER du worker avant le premier appel à frankenphp_handle_request()
+$workerServer = $_SERVER;
+
+$handler = static function () use ($workerServer) {
+    var_dump($_SERVER); // $_SERVER lié à la requête
+    var_dump($workerServer); // $_SERVER du script worker
+};
+
+// ...
+```

docs/tr/worker.md

@@ -71,12 +71,12 @@ $myApp->boot();
 
 // Daha iyi performans için döngü dışında işleyici (daha az iş yapıyor)
 $handler = static function () use ($myApp) {
-        // Bir istek alındığında çağrılır,
-        // superglobals, php://input ve benzerleri sıfırlanır
-        echo $myApp->handle($_GET, $_POST, $_COOKIE, $_FILES, $_SERVER);
+    // Bir istek alındığında çağrılır,
+    // superglobals, php://input ve benzerleri sıfırlanır
+    echo $myApp->handle($_GET, $_POST, $_COOKIE, $_FILES, $_SERVER);
 };
 
-for($nbRequests = 0, $running = true; isset($_SERVER['MAX_REQUESTS']) && ($nbRequests < ((int)$_SERVER['MAX_REQUESTS'])) && $running; ++$nbRequests) {
+for ($nbRequests = 0, $running = true; isset($_SERVER['MAX_REQUESTS']) && ($nbRequests < ((int)$_SERVER['MAX_REQUESTS'])) && $running; ++$nbRequests) {
     $running = \frankenphp_handle_request($handler);
 
     // HTTP yanıtını gönderdikten sonra bir şey yapın

docs/worker.md

@@ -71,12 +71,12 @@ $myApp->boot();
 
 // Handler outside the loop for better performance (doing less work)
 $handler = static function () use ($myApp) {
-        // Called when a request is received,
-        // superglobals, php://input and the like are reset
-        echo $myApp->handle($_GET, $_POST, $_COOKIE, $_FILES, $_SERVER);
+    // Called when a request is received,
+    // superglobals, php://input and the like are reset
+    echo $myApp->handle($_GET, $_POST, $_COOKIE, $_FILES, $_SERVER);
 };
 
-for($nbRequests = 0, $running = true; isset($_SERVER['MAX_REQUESTS']) && ($nbRequests < ((int)$_SERVER['MAX_REQUESTS'])) && $running; ++$nbRequests) {
+for ($nbRequests = 0, $running = true; isset($_SERVER['MAX_REQUESTS']) && ($nbRequests < ((int) $_SERVER['MAX_REQUESTS'])) && $running; ++$nbRequests) {
     $running = \frankenphp_handle_request($handler);
 
     // Do something after sending the HTTP response
@@ -117,3 +117,26 @@ As PHP was not originally designed for long-running processes, there are still m
 A workaround to using this type of code in worker mode is to restart the worker script after processing a certain number of requests:
 
 The previous worker snippet allows configuring a maximum number of request to handle by setting an environment variable named `MAX_REQUESTS`.
+
+## Superglobals Behavior
+
+[PHP superglobals](https://www.php.net/manual/en/language.variables.superglobals.php) (`$_SERVER`, `$_ENV`, `$_GET`...)
+behave as follow:
+
+* before the first call to `frankenphp_handle_request()`, superglobals contain values bound to the worker script itself
+* during and after the call to `frankenphp_handle_request()`, superglobals contain values generated from the processed HTTP request, each call to `frankenphp_handle_request()` changes the superglobals values
+
+To access the superglobals of the worker script inside the callback, you must copy them and import the copy in the scope of the callback:
+
+```php
+<?php
+// Copy worker's $_SERVER superglobal before the first call to frankenphp_handle_request()
+$workerServer = $_SERVER;
+
+$handler = static function () use ($workerServer) {
+    var_dump($_SERVER); // Request-bound $_SERVER
+    var_dump($workerServer); // $_SERVER of the worker script
+};
+
+// ...
+```

frankenphp.c

@@ -73,11 +73,11 @@ typedef struct frankenphp_server_context {
   bool finished;
 } frankenphp_server_context;
 
-static uintptr_t frankenphp_clean_server_context() {
+static void frankenphp_free_request_context() {
   frankenphp_server_context *ctx = SG(server_context);
-  if (ctx == NULL) {
-    return 0;
-  }
+
+  free(ctx->cookie_data);
+  ctx->cookie_data = NULL;
 
   free(SG(request_info).auth_password);
   SG(request_info).auth_password = NULL;
@@ -99,19 +99,13 @@ static uintptr_t frankenphp_clean_server_context() {
 
   free(SG(request_info).request_uri);
   SG(request_info).request_uri = NULL;
-
-  return ctx->current_request;
 }
 
-static void frankenphp_request_reset() {
+static void frankenphp_destroy_super_globals() {
   zend_try {
-    int i;
-
-    for (i = 0; i < NUM_TRACK_VARS; i++) {
-      zval_ptr_dtor(&PG(http_globals)[i]);
+    for (int i = 0; i < NUM_TRACK_VARS; i++) {
+      zval_ptr_dtor_nogc(&PG(http_globals)[i]);
     }
-
-    memset(&PG(http_globals), 0, sizeof(zval) * NUM_TRACK_VARS);
   }
   zend_end_try();
 }
@@ -137,11 +131,8 @@ static void frankenphp_worker_request_shutdown() {
   zend_try { php_output_deactivate(); }
   zend_end_try();
 
-  /* Clean super globals */
-  frankenphp_request_reset();
-
   /* SAPI related shutdown (free stuff) */
-  frankenphp_clean_server_context();
+  frankenphp_free_request_context();
   zend_try { sapi_deactivate(); }
   zend_end_try();
 
@@ -153,6 +144,7 @@ static int frankenphp_worker_request_startup() {
   int retval = SUCCESS;
 
   zend_try {
+    frankenphp_destroy_super_globals();
     php_output_activate();
 
     /* initialize global variables */
@@ -419,28 +411,22 @@ static zend_module_entry frankenphp_module = {
     TOSTRING(FRANKENPHP_VERSION),
     STANDARD_MODULE_PROPERTIES};
 
-static uintptr_t frankenphp_request_shutdown() {
+static void frankenphp_request_shutdown() {
   frankenphp_server_context *ctx = SG(server_context);
 
   if (ctx->main_request && ctx->current_request) {
-    frankenphp_request_reset();
+    frankenphp_destroy_super_globals();
   }
 
   php_request_shutdown((void *)0);
-
-  free(ctx->cookie_data);
-  ((frankenphp_server_context *)SG(server_context))->cookie_data = NULL;
-  uintptr_t rh = frankenphp_clean_server_context();
+  frankenphp_free_request_context();
 
   free(ctx);
-  SG(server_context) = NULL;
-  ctx = NULL;
+  SG(server_context) = ctx = NULL;
 
 #if defined(ZTS)
   ts_free_thread();
 #endif
-
-  return rh;
 }
 
 int frankenphp_update_server_context(
@@ -856,7 +842,7 @@ int frankenphp_execute_script(char *file_name) {
 
   zend_destroy_file_handle(&file_handle);
 
-  frankenphp_clean_server_context();
+  frankenphp_free_request_context();
   frankenphp_request_shutdown();
 
   return status;

frankenphp.go

@@ -755,7 +755,7 @@ func go_read_cookies(rh C.uintptr_t) *C.char {
 		cookieStrings[i] = cookie.String()
 	}
 
-	// freed in frankenphp_request_shutdown()
+	// freed in frankenphp_free_request_context()
 	return C.CString(strings.Join(cookieStrings, "; "))
 }
 

testdata/worker-getopt.php

@@ -0,0 +1,19 @@
+<?php
+
+do {
+    $ok = frankenphp_handle_request(function (): void {
+        print_r($_SERVER);
+    });
+
+    getopt('abc');
+
+    if (!isset($_SERVER['HTTP_REQUEST'])) {
+        exit(1);
+    }
+    if (isset($_SERVER['FRANKENPHP_WORKER'])) {
+        exit(2);
+    }
+    if (isset($_SERVER['FOO'])) {
+        exit(3);
+    }
+} while ($ok);

worker_test.go

@@ -7,11 +7,14 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strconv"
 	"strings"
 	"testing"
 
 	"github.com/dunglas/frankenphp"
 	"github.com/stretchr/testify/assert"
+	"go.uber.org/zap"
+	"go.uber.org/zap/zaptest/observer"
 )
 
 func TestWorker(t *testing.T) {
@@ -88,6 +91,29 @@ func TestWorkerEnv(t *testing.T) {
 	}, &testOptions{workerScript: "env.php", nbWorkers: 1, env: map[string]string{"FOO": "bar"}, nbParrallelRequests: 10})
 }
 
+func TestWorkerGetOpt(t *testing.T) {
+	observer, logs := observer.New(zap.InfoLevel)
+	logger := zap.New(observer)
+
+	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) {
+		req := httptest.NewRequest("GET", fmt.Sprintf("http://example.com/worker-getopt.php?i=%d", i), nil)
+		req.Header.Add("Request", strconv.Itoa(i))
+		w := httptest.NewRecorder()
+
+		handler(w, req)
+
+		resp := w.Result()
+		body, _ := io.ReadAll(resp.Body)
+
+		assert.Contains(t, string(body), fmt.Sprintf("[HTTP_REQUEST] => %d", i))
+		assert.Contains(t, string(body), fmt.Sprintf("[REQUEST_URI] => /worker-getopt.php?i=%d", i))
+	}, &testOptions{logger: logger, workerScript: "worker-getopt.php", env: map[string]string{"FOO": "bar"}})
+
+	for _, log := range logs.FilterFieldKey("exit_status").All() {
+		assert.Failf(t, "unexpected exit status", "exit status: %d", log.ContextMap()["exit_status"])
+	}
+}
+
 func ExampleServeHTTP_workers() {
 	if err := frankenphp.Init(
 		frankenphp.WithWorkers("worker1.php", 4, map[string]string{"ENV1": "foo"}),