fix: don't flush env between requests (#1814)

Author: AlliBalliBaba

frankenphp.c

@@ -101,13 +101,44 @@ static void frankenphp_free_request_context() {
   SG(request_info).request_uri = NULL;
 }
 
-static void frankenphp_destroy_super_globals() {
+/* reset all 'auto globals' in worker mode except of $_ENV
+ * see: php_hash_environment() */
+static void frankenphp_reset_super_globals() {
   zend_try {
-    for (int i = 0; i < NUM_TRACK_VARS; i++) {
-      zval_ptr_dtor_nogc(&PG(http_globals)[i]);
-    }
+    /* only $_FILES needs to be flushed explicitly
+     * $_GET, $_POST, $_COOKIE and $_SERVER are flushed on reimport
+     * $_ENV is not flushed
+     * for more info see: php_startup_auto_globals()
+     */
+    zval *files = &PG(http_globals)[TRACK_VARS_FILES];
+    zval_ptr_dtor_nogc(files);
+    memset(files, 0, sizeof(*files));
   }
   zend_end_try();
+
+  zend_auto_global *auto_global;
+  zend_string *_env = ZSTR_KNOWN(ZEND_STR_AUTOGLOBAL_ENV);
+  zend_string *_server = ZSTR_KNOWN(ZEND_STR_AUTOGLOBAL_SERVER);
+  ZEND_HASH_MAP_FOREACH_PTR(CG(auto_globals), auto_global) {
+    if (auto_global->name == _env) {
+      /* skip $_ENV */
+    } else if (auto_global->name == _server) {
+      /* always reimport $_SERVER  */
+      auto_global->armed = auto_global->auto_global_callback(auto_global->name);
+    } else if (auto_global->jit) {
+      /* globals with jit are: $_SERVER, $_ENV, $_REQUEST, $GLOBALS,
+       * jit will only trigger on script parsing and therefore behaves
+       * differently in worker mode. We will skip all jit globals
+       */
+    } else if (auto_global->auto_global_callback) {
+      /* $_GET, $_POST, $_COOKIE, $_FILES are reimported here */
+      auto_global->armed = auto_global->auto_global_callback(auto_global->name);
+    } else {
+      /* $_SESSION will land here (not an http_global) */
+      auto_global->armed = 0;
+    }
+  }
+  ZEND_HASH_FOREACH_END();
 }
 
 /*
@@ -187,7 +218,6 @@ static int frankenphp_worker_request_startup() {
   frankenphp_update_request_context();
 
   zend_try {
-    frankenphp_destroy_super_globals();
     frankenphp_release_temporary_streams();
     php_output_activate();
 
@@ -225,21 +255,7 @@ static int frankenphp_worker_request_startup() {
       php_output_set_implicit_flush(1);
     }
 
-    php_hash_environment();
-
-    /* zend_is_auto_global will force a re-import of the $_SERVER global */
-    zend_is_auto_global(ZSTR_KNOWN(ZEND_STR_AUTOGLOBAL_SERVER));
-
-    /* disarm the $_ENV auto_global to prevent it from being reloaded in worker
-     * mode */
-    if (zend_hash_str_exists(&EG(symbol_table), "_ENV", 4)) {
-      zend_auto_global *env_global;
-      if ((env_global = zend_hash_find_ptr(
-               CG(auto_globals), ZSTR_KNOWN(ZEND_STR_AUTOGLOBAL_ENV))) !=
-          NULL) {
-        env_global->armed = 0;
-      }
-    }
+    frankenphp_reset_super_globals();
 
     const char **module_name;
     zend_module_entry *module;
@@ -514,11 +530,6 @@ static zend_module_entry frankenphp_module = {
     STANDARD_MODULE_PROPERTIES};
 
 static void frankenphp_request_shutdown() {
-  if (is_worker_thread) {
-    /* ensure $_ENV is not in an invalid state before shutdown */
-    zval_ptr_dtor_nogc(&PG(http_globals)[TRACK_VARS_ENV]);
-    array_init(&PG(http_globals)[TRACK_VARS_ENV]);
-  }
   frankenphp_free_request_context();
   php_request_shutdown((void *)0);
 }

internal/phpheaders/phpheaders_test.go

@@ -12,6 +12,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/dunglas/frankenphp/internal/phpheaders"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -235,6 +236,20 @@ func allPossibleTransitions(worker1Path string, worker2Path string) []func(*phpT
 	}
 }
 
+func TestAllCommonHeadersAreCorrect(t *testing.T) {
+	fakeRequest := httptest.NewRequest("GET", "http://localhost", nil)
+
+	for header, phpHeader := range phpheaders.CommonRequestHeaders {
+		// verify that common and uncommon headers return the same result
+		expectedPHPHeader := phpheaders.GetUnCommonHeader(header)
+		assert.Equal(t, phpHeader+"\x00", expectedPHPHeader, "header is not well formed: "+phpHeader)
+
+		// net/http will capitalize lowercase headers, verify that headers are capitalized
+		fakeRequest.Header.Add(header, "foo")
+		assert.Contains(t, fakeRequest.Header, header, "header is not correctly capitalized: "+header)
+	}
+}
+
 func TestCorrectThreadCalculation(t *testing.T) {
 	maxProcs := runtime.GOMAXPROCS(0) * 2
 	oneWorkerThread := []workerOpt{{num: 1}}