fix: concurrent env access (#1409)

Author: AlliBalliBaba

caddy/caddy_test.go

@@ -716,3 +716,35 @@ func testSingleIniConfiguration(tester *caddytest.Tester, key string, value stri
 		)
 	}
 }
+
+func TestOsEnv(t *testing.T) {
+	os.Setenv("ENV1", "value1")
+	os.Setenv("ENV2", "value2")
+	tester := caddytest.NewTester(t)
+	tester.InitServer(`
+		{
+			skip_install_trust
+			admin localhost:2999
+			http_port `+testPort+`
+
+			frankenphp {
+				num_threads 2
+				php_ini variables_order "EGPCS"
+				worker ../testdata/env/env.php 1
+			}
+		}
+
+		localhost:`+testPort+` {
+			route {
+				root ../testdata
+				php
+			}
+		}
+		`, "caddyfile")
+
+	tester.AssertGetResponse(
+		"http://localhost:"+testPort+"/env/env.php?keys[]=ENV1&keys[]=ENV2",
+		http.StatusOK,
+		"ENV1=value1,ENV2=value2",
+	)
+}

env.go

@@ -1,5 +1,9 @@
 package frankenphp
 
+// #cgo nocallback frankenphp_init_persistent_string
+// #cgo nocallback frankenphp_add_assoc_str_ex
+// #cgo noescape frankenphp_init_persistent_string
+// #cgo noescape frankenphp_add_assoc_str_ex
 // #include "frankenphp.h"
 import "C"
 import (
@@ -8,67 +12,106 @@ import (
 	"unsafe"
 )
 
+func initializeEnv() map[string]*C.zend_string {
+	env := os.Environ()
+	envMap := make(map[string]*C.zend_string, len(env))
+
+	for _, envVar := range env {
+		key, val, _ := strings.Cut(envVar, "=")
+		envMap[key] = C.frankenphp_init_persistent_string(toUnsafeChar(val), C.size_t(len(val)))
+	}
+
+	return envMap
+}
+
+// get the main thread env or the thread specific env
+func getSandboxedEnv(thread *phpThread) map[string]*C.zend_string {
+	if thread.sandboxedEnv != nil {
+		return thread.sandboxedEnv
+	}
+
+	return mainThread.sandboxedEnv
+}
+
+func clearSandboxedEnv(thread *phpThread) {
+	if thread.sandboxedEnv == nil {
+		return
+	}
+
+	for key, val := range thread.sandboxedEnv {
+		valInMainThread, ok := mainThread.sandboxedEnv[key]
+		if !ok || val != valInMainThread {
+			C.free(unsafe.Pointer(val))
+		}
+	}
+
+	thread.sandboxedEnv = nil
+}
+
+// if an env var already exists, it needs to be freed
+func removeEnvFromThread(thread *phpThread, key string) {
+	valueInThread, existsInThread := thread.sandboxedEnv[key]
+	if !existsInThread {
+		return
+	}
+
+	valueInMainThread, ok := mainThread.sandboxedEnv[key]
+	if !ok || valueInThread != valueInMainThread {
+		C.free(unsafe.Pointer(valueInThread))
+	}
+
+	delete(thread.sandboxedEnv, key)
+}
+
+// copy the main thread env to the thread specific env
+func cloneSandboxedEnv(thread *phpThread) {
+	if thread.sandboxedEnv != nil {
+		return
+	}
+	thread.sandboxedEnv = make(map[string]*C.zend_string, len(mainThread.sandboxedEnv))
+	for key, value := range mainThread.sandboxedEnv {
+		thread.sandboxedEnv[key] = value
+	}
+}
+
 //export go_putenv
-func go_putenv(str *C.char, length C.int) C.bool {
+func go_putenv(threadIndex C.uintptr_t, str *C.char, length C.int) C.bool {
+	thread := phpThreads[threadIndex]
 	envString := C.GoStringN(str, length)
+	cloneSandboxedEnv(thread)
 
 	// Check if '=' is present in the string
 	if key, val, found := strings.Cut(envString, "="); found {
+		removeEnvFromThread(thread, key)
+		thread.sandboxedEnv[key] = C.frankenphp_init_persistent_string(toUnsafeChar(val), C.size_t(len(val)))
 		return os.Setenv(key, val) == nil
 	}
 
 	// No '=', unset the environment variable
+	removeEnvFromThread(thread, envString)
 	return os.Unsetenv(envString) == nil
 }
 
 //export go_getfullenv
-func go_getfullenv(threadIndex C.uintptr_t) (*C.go_string, C.size_t) {
+func go_getfullenv(threadIndex C.uintptr_t, trackVarsArray *C.zval) {
 	thread := phpThreads[threadIndex]
+	env := getSandboxedEnv(thread)
 
-	env := os.Environ()
-	goStrings := make([]C.go_string, len(env)*2)
-
-	for i, envVar := range env {
-		key, val, _ := strings.Cut(envVar, "=")
-		goStrings[i*2] = C.go_string{C.size_t(len(key)), thread.pinString(key)}
-		goStrings[i*2+1] = C.go_string{C.size_t(len(val)), thread.pinString(val)}
+	for key, val := range env {
+		C.frankenphp_add_assoc_str_ex(trackVarsArray, toUnsafeChar(key), C.size_t(len(key)), val)
 	}
-
-	value := unsafe.SliceData(goStrings)
-	thread.Pin(value)
-
-	return value, C.size_t(len(env))
 }
 
 //export go_getenv
-func go_getenv(threadIndex C.uintptr_t, name *C.go_string) (C.bool, *C.go_string) {
+func go_getenv(threadIndex C.uintptr_t, name *C.char) (C.bool, *C.zend_string) {
 	thread := phpThreads[threadIndex]
 
-	// Create a byte slice from C string with a specified length
-	envName := C.GoStringN(name.data, C.int(name.len))
-
 	// Get the environment variable value
-	envValue, exists := os.LookupEnv(envName)
+	envValue, exists := getSandboxedEnv(thread)[C.GoString(name)]
 	if !exists {
 		// Environment variable does not exist
 		return false, nil // Return 0 to indicate failure
 	}
 
-	// Convert Go string to C string
-	value := &C.go_string{C.size_t(len(envValue)), thread.pinString(envValue)}
-	thread.Pin(value)
-
-	return true, value // Return 1 to indicate success
-}
-
-//export go_sapi_getenv
-func go_sapi_getenv(threadIndex C.uintptr_t, name *C.go_string) *C.char {
-	envName := C.GoStringN(name.data, C.int(name.len))
-
-	envValue, exists := os.LookupEnv(envName)
-	if !exists {
-		return nil
-	}
-
-	return phpThreads[threadIndex].pinCString(envValue)
+	return true, envValue // Return 1 to indicate success
 }

frankenphp.c

@@ -160,18 +160,12 @@ static void frankenphp_worker_request_shutdown() {
 }
 
 PHPAPI void get_full_env(zval *track_vars_array) {
-  struct go_getfullenv_return full_env = go_getfullenv(thread_index);
-
-  for (int i = 0; i < full_env.r1; i++) {
-    go_string key = full_env.r0[i * 2];
-    go_string val = full_env.r0[i * 2 + 1];
-
-    // create PHP string for the value
-    zend_string *val_str = zend_string_init(val.data, val.len, 0);
+  go_getfullenv(thread_index, track_vars_array);
+}
 
-    // add to the associative array
-    add_assoc_str_ex(track_vars_array, key.data, key.len, val_str);
-  }
+void frankenphp_add_assoc_str_ex(zval *track_vars_array, char *key,
+                                 size_t keylen, zend_string *val) {
+  add_assoc_str_ex(track_vars_array, key, keylen, val);
 }
 
 /* Adapted from php_request_startup() */
@@ -219,8 +213,20 @@ static int frankenphp_worker_request_startup() {
 
     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;
+      }
+    }
+
     /* Unfinish the request */
     frankenphp_server_context *ctx = SG(server_context);
     ctx->finished = false;
@@ -282,7 +288,7 @@ PHP_FUNCTION(frankenphp_putenv) {
     RETURN_FALSE;
   }
 
-  if (go_putenv(setting, (int)setting_len)) {
+  if (go_putenv(thread_index, setting, (int)setting_len)) {
     RETURN_TRUE;
   } else {
     RETURN_FALSE;
@@ -308,13 +314,11 @@ PHP_FUNCTION(frankenphp_getenv) {
     return;
   }
 
-  go_string gname = {name_len, name};
-
-  struct go_getenv_return result = go_getenv(thread_index, &gname);
+  struct go_getenv_return result = go_getenv(thread_index, name);
 
   if (result.r0) {
     // Return the single environment variable as a string
-    RETVAL_STRINGL(result.r1->data, result.r1->len);
+    RETVAL_STR(result.r1);
   } else {
     // Environment variable does not exist
     RETVAL_FALSE;
@@ -748,17 +752,14 @@ void frankenphp_register_bulk(
 zend_string *frankenphp_init_persistent_string(const char *string, size_t len) {
   /* persistent strings will be ignored by the GC at the end of a request */
   zend_string *z_string = zend_string_init(string, len, 1);
+  zend_string_hash_val(z_string);
 
   /* interned strings will not be ref counted by the GC */
   GC_ADD_FLAGS(z_string, IS_STR_INTERNED);
 
   return z_string;
 }
 
-void frankenphp_release_zend_string(zend_string *z_string) {
-  zend_string_release(z_string);
-}
-
 static void
 frankenphp_register_variable_from_request_info(zend_string *zKey, char *value,
                                                bool must_be_present,
@@ -844,9 +845,13 @@ static void frankenphp_log_message(const char *message, int syslog_type_int) {
 }
 
 static char *frankenphp_getenv(const char *name, size_t name_len) {
-  go_string gname = {name_len, (char *)name};
+  struct go_getenv_return result = go_getenv(thread_index, (char *)name);
 
-  return go_sapi_getenv(thread_index, &gname);
+  if (result.r0) {
+    return result.r1->val;
+  }
+
+  return NULL;
 }
 
 sapi_module_struct frankenphp_sapi_module = {

frankenphp.h

@@ -69,9 +69,10 @@ void frankenphp_register_variables_from_request_info(
 void frankenphp_register_variable_safe(char *key, char *var, size_t val_len,
                                        zval *track_vars_array);
 zend_string *frankenphp_init_persistent_string(const char *string, size_t len);
-void frankenphp_release_zend_string(zend_string *z_string);
 int frankenphp_reset_opcache(void);
 int frankenphp_get_current_memory_limit();
+void frankenphp_add_assoc_str_ex(zval *track_vars_array, char *key,
+                                 size_t keylen, zend_string *val);
 
 void frankenphp_register_single(zend_string *z_key, char *value, size_t val_len,
                                 zval *track_vars_array);

frankenphp_test.go

@@ -45,6 +45,7 @@ type testOptions struct {
 	realServer         bool
 	logger             *zap.Logger
 	initOpts           []frankenphp.Option
+	phpIni             map[string]string
 }
 
 func runTest(t *testing.T, test func(func(http.ResponseWriter, *http.Request), *httptest.Server, int), opts *testOptions) {
@@ -67,6 +68,9 @@ func runTest(t *testing.T, test func(func(http.ResponseWriter, *http.Request), *
 		initOpts = append(initOpts, frankenphp.WithWorkers(testDataDir+opts.workerScript, opts.nbWorkers, opts.env, opts.watch))
 	}
 	initOpts = append(initOpts, opts.initOpts...)
+	if opts.phpIni != nil {
+		initOpts = append(initOpts, frankenphp.WithPhpIni(opts.phpIni))
+	}
 
 	err := frankenphp.Init(initOpts...)
 	require.Nil(t, err)
@@ -671,21 +675,21 @@ func TestEnv(t *testing.T) {
 	testEnv(t, &testOptions{})
 }
 func TestEnvWorker(t *testing.T) {
-	testEnv(t, &testOptions{workerScript: "test-env.php"})
+	testEnv(t, &testOptions{workerScript: "env/test-env.php"})
 }
 func testEnv(t *testing.T, opts *testOptions) {
 	assert.NoError(t, os.Setenv("EMPTY", ""))
 
 	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) {
-		req := httptest.NewRequest("GET", fmt.Sprintf("http://example.com/test-env.php?var=%d", i), nil)
+		req := httptest.NewRequest("GET", fmt.Sprintf("http://example.com/env/test-env.php?var=%d", i), nil)
 		w := httptest.NewRecorder()
 		handler(w, req)
 
 		resp := w.Result()
 		body, _ := io.ReadAll(resp.Body)
 
 		// execute the script as regular php script
-		cmd := exec.Command("php", "testdata/test-env.php", strconv.Itoa(i))
+		cmd := exec.Command("php", "testdata/env/test-env.php", strconv.Itoa(i))
 		stdoutStderr, err := cmd.CombinedOutput()
 		if err != nil {
 			// php is not installed or other issue, use the hardcoded output below:
@@ -696,6 +700,46 @@ func testEnv(t *testing.T, opts *testOptions) {
 	}, opts)
 }
 
+func TestEnvIsResetInNonWorkerMode(t *testing.T) {
+	assert.NoError(t, os.Setenv("test", ""))
+	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) {
+		putResult := fetchBody("GET", fmt.Sprintf("http://example.com/env/putenv.php?key=test&put=%d", i), handler)
+
+		assert.Equal(t, fmt.Sprintf("test=%d", i), putResult, "putenv and then echo getenv")
+
+		getResult := fetchBody("GET", "http://example.com/env/putenv.php?key=test", handler)
+
+		assert.Equal(t, "test=", getResult, "putenv should be reset across requests")
+	}, &testOptions{})
+}
+
+// TODO: should it actually get reset in worker mode?
+func TestEnvIsNotResetInWorkerMode(t *testing.T) {
+	assert.NoError(t, os.Setenv("index", ""))
+	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) {
+		putResult := fetchBody("GET", fmt.Sprintf("http://example.com/env/remember-env.php?index=%d", i), handler)
+
+		assert.Equal(t, "success", putResult, "putenv and then echo getenv")
+
+		getResult := fetchBody("GET", "http://example.com/env/remember-env.php", handler)
+
+		assert.Equal(t, "success", getResult, "putenv should not be reset across worker requests")
+	}, &testOptions{workerScript: "env/remember-env.php"})
+}
+
+// reproduction of https://github.com/dunglas/frankenphp/issues/1061
+func TestModificationsToEnvPersistAcrossRequests(t *testing.T) {
+	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) {
+		for j := 0; j < 3; j++ {
+			result := fetchBody("GET", "http://example.com/env/overwrite-env.php", handler)
+			assert.Equal(t, "custom_value", result, "a var directly added to $_ENV should persist")
+		}
+	}, &testOptions{
+		workerScript: "env/overwrite-env.php",
+		phpIni:       map[string]string{"variables_order": "EGPCS"},
+	})
+}
+
 func TestFileUpload_module(t *testing.T) { testFileUpload(t, &testOptions{}) }
 func TestFileUpload_worker(t *testing.T) {
 	testFileUpload(t, &testOptions{workerScript: "file-upload.php"})