fix: concurrent env access

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"})