diff --git a/env.go b/env.go index d1e464dfb..2dca69000 100644 --- a/env.go +++ b/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 } diff --git a/frankenphp.c b/frankenphp.c index 0d8e1e961..ac4bef883 100644 --- a/frankenphp.c +++ b/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 = { diff --git a/frankenphp.h b/frankenphp.h index b8c255040..0365bb3cb 100644 --- a/frankenphp.h +++ b/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); diff --git a/frankenphp_test.go b/frankenphp_test.go index 5f0d745e4..3995f1051 100644 --- a/frankenphp_test.go +++ b/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"}) diff --git a/metrics.go b/metrics.go index ba9bfb942..2d87199a6 100644 --- a/metrics.go +++ b/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 } diff --git a/phpmainthread.go b/phpmainthread.go index dfb753be9..25ebe198c 100644 --- a/phpmainthread.go +++ b/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 diff --git a/phpthread.go b/phpthread.go index bd88e6b01..927f54377 100644 --- a/phpthread.go +++ b/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, } } diff --git a/testdata/env/env-global-import.php b/testdata/env/env-global-import.php new file mode 100644 index 000000000..dd610dd59 --- /dev/null +++ b/testdata/env/env-global-import.php @@ -0,0 +1,3 @@ +