fix: concurrent env access

env.go

@@ -1,74 +1,98 @@
 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 (
 	"os"
 	"strings"
-	"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
+}
+
+// copy the main thread env to the thread specific env
+func requireSandboxedEnv(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)
+	requireSandboxedEnv(thread)
 
 	// Check if '=' is present in the string
 	if key, val, found := strings.Cut(envString, "="); found {
+		// TODO: free this zend_string (if it exists and after requests)
+		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
+	delete(thread.sandboxedEnv, 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
+	return true, envValue // 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))
+func go_sapi_getenv(threadIndex C.uintptr_t, name *C.char) *C.zend_string {
+	thread := phpThreads[threadIndex]
 
-	envValue, exists := os.LookupEnv(envName)
+	envValue, exists := getSandboxedEnv(thread)[C.GoString(name)]
 	if !exists {
 		return nil
 	}
 
-	return phpThreads[threadIndex].pinCString(envValue)
+	return envValue
 }

frankenphp.c

@@ -160,18 +160,26 @@ 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];
+  /* if the array is the $_ENV global and it already exists,
+     load all of the previous entries
+     this ensures $_ENV is not unexpectedly reloaded when
+     compiling a new script in worker mode */
+  if (track_vars_array == &PG(http_globals)[TRACK_VARS_ENV] &&
+      zend_hash_str_exists(&EG(symbol_table), "_ENV", 4)) {
+    zval *env = zend_hash_str_find(&EG(symbol_table), "_ENV", 4);
+    zend_hash_copy(Z_ARR_P(track_vars_array), Z_ARR_P(env),
+                   (copy_ctor_func_t)zval_add_ref);
+    return;
+  }
 
-    // create PHP string for the value
-    zend_string *val_str = zend_string_init(val.data, val.len, 0);
+  /* in all other cases, get the env from go */
+  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,6 +227,7 @@ 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));
 
     /* Unfinish the request */
@@ -282,7 +291,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 +317,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,6 +755,7 @@ 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);
@@ -844,9 +852,7 @@ 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};
-
-  return go_sapi_getenv(thread_index, &gname);
+  return go_sapi_getenv(thread_index, (char *)(name))->val;
 }
 
 sapi_module_struct frankenphp_sapi_module = {

frankenphp.h

@@ -72,6 +72,7 @@ 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)
@@ -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{nbParallelRequests: 20})
+}
+
+// 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/env-index.php?index=%d", i), handler)
+
+		assert.Equal(t, "success", putResult, "putenv and then echo getenv")
+
+		getResult := fetchBody("GET", "http://example.com/env/env-index.php", handler)
+
+		assert.Equal(t, "success", getResult, "putenv should not be reset across worker requests")
+	}, &testOptions{workerScript: "env/env-index.php", nbParallelRequests: 20})
+}
+
+// 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/env-global.php", handler)
+			assert.Equal(t, "custom_value", result, "a var directly added to $_ENV should persist")
+		}
+	}, &testOptions{
+		workerScript: "env/env-global.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"})

metrics.go

@@ -380,9 +380,9 @@ func (m *PrometheusMetrics) Shutdown() {
 	}
 
 	if err := m.registry.Register(m.queueDepth); err != nil &&
-        !errors.As(err, &prometheus.AlreadyRegisteredError{}) {
-        panic(err)
-    }
+		!errors.As(err, &prometheus.AlreadyRegisteredError{}) {
+		panic(err)
+	}
 }
 
 func getWorkerNameForMetrics(name string) string {
@@ -432,9 +432,9 @@ func NewPrometheusMetrics(registry prometheus.Registerer) *PrometheusMetrics {
 	}
 
 	if err := m.registry.Register(m.queueDepth); err != nil &&
-        !errors.As(err, &prometheus.AlreadyRegisteredError{}) {
-        panic(err)
-    }
+		!errors.As(err, &prometheus.AlreadyRegisteredError{}) {
+		panic(err)
+	}
 
 	return m
 }

phpmainthread.go

@@ -22,6 +22,7 @@ type phpMainThread struct {
 	phpIni          map[string]string
 	commonHeaders   map[string]*C.zend_string
 	knownServerKeys map[string]*C.zend_string
+	sandboxedEnv    map[string]*C.zend_string
 }
 
 var (
@@ -34,11 +35,12 @@ var (
 // and reserves a fixed number of possible PHP threads
 func initPHPThreads(numThreads int, numMaxThreads int, phpIni map[string]string) (*phpMainThread, error) {
 	mainThread = &phpMainThread{
-		state:      newThreadState(),
-		done:       make(chan struct{}),
-		numThreads: numThreads,
-		maxThreads: numMaxThreads,
-		phpIni:     phpIni,
+		state:        newThreadState(),
+		done:         make(chan struct{}),
+		numThreads:   numThreads,
+		maxThreads:   numMaxThreads,
+		phpIni:       phpIni,
+		sandboxedEnv: initializeEnv(),
 	}
 
 	// initialize the first thread

phpthread.go

@@ -15,12 +15,13 @@ import (
 // identified by the index in the phpThreads slice
 type phpThread struct {
 	runtime.Pinner
-	threadIndex int
-	requestChan chan *http.Request
-	drainChan   chan struct{}
-	handlerMu   sync.Mutex
-	handler     threadHandler
-	state       *threadState
+	threadIndex  int
+	requestChan  chan *http.Request
+	drainChan    chan struct{}
+	handlerMu    sync.Mutex
+	handler      threadHandler
+	state        *threadState
+	sandboxedEnv map[string]*C.zend_string
 }
 
 // interface that defines how the callbacks from the C thread should be handled
@@ -33,9 +34,10 @@ type threadHandler interface {
 
 func newPHPThread(threadIndex int) *phpThread {
 	return &phpThread{
-		threadIndex: threadIndex,
-		requestChan: make(chan *http.Request),
-		state:       newThreadState(),
+		threadIndex:  threadIndex,
+		requestChan:  make(chan *http.Request),
+		state:        newThreadState(),
+		sandboxedEnv: nil,
 	}
 }
 

testdata/env/env-global-import.php

@@ -0,0 +1,3 @@
+<?php
+
+return $_ENV['custom_key'];

testdata/env/env-global.php

@@ -0,0 +1,10 @@
+<?php
+
+require_once __DIR__.'/../_executor.php';
+
+$_ENV['custom_key'] = 'custom_value';
+
+return function () use (&$rememberedIndex) {
+    $custom_key = require __DIR__.'/env-global-import.php';
+    echo $custom_key;
+};