diff --git a/caddy/caddy_test.go b/caddy/caddy_test.go index 4233fc3111..037ccc4715 100644 --- a/caddy/caddy_test.go +++ b/caddy/caddy_test.go @@ -21,6 +21,16 @@ import ( var testPort = "9080" +func TestMain(m *testing.M) { + // setup custom environment vars for TestOsEnv + if os.Setenv("ENV1", "value1") != nil || os.Setenv("ENV2", "value2") != nil { + fmt.Println("Failed to set environment variables for tests") + os.Exit(1) + } + + os.Exit(m.Run()) +} + func TestPHP(t *testing.T) { var wg sync.WaitGroup tester := caddytest.NewTester(t) @@ -904,8 +914,6 @@ 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(` { diff --git a/cgi_test.go b/cgi_test.go index d7c0e854d7..c4c7a7701c 100644 --- a/cgi_test.go +++ b/cgi_test.go @@ -25,7 +25,7 @@ func TestEnsureLeadingSlash(t *testing.T) { } for _, tt := range tests { - t.Run(tt.input + "-" + tt.expected, func(t *testing.T) { + t.Run(tt.input+"-"+tt.expected, func(t *testing.T) { t.Parallel() assert.Equal(t, tt.expected, ensureLeadingSlash(tt.input), "ensureLeadingSlash(%q)", tt.input) diff --git a/env.go b/env.go index 3ac9a3adf6..6e99f1769a 100644 --- a/env.go +++ b/env.go @@ -3,114 +3,32 @@ package frankenphp // #cgo nocallback frankenphp_init_persistent_string // #cgo noescape frankenphp_init_persistent_string // #include "frankenphp.h" -// #include +// #include "types.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 { +//export go_init_os_env +func go_init_os_env(mainThreadEnv *C.zend_array) { + for _, envVar := range os.Environ() { 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 + zkey := C.frankenphp_init_persistent_string(toUnsafeChar(key), C.size_t(len(key))) + zStr := C.frankenphp_init_persistent_string(toUnsafeChar(val), C.size_t(len(val))) + C.__hash_update_string__(mainThreadEnv, zkey, zStr) } } //export go_putenv -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, trackVarsArray *C.zval) { - thread := phpThreads[threadIndex] - env := getSandboxedEnv(thread) - - for key, val := range env { - C.add_assoc_str_ex(trackVarsArray, toUnsafeChar(key), C.size_t(len(key)), val) - } -} - -//export go_getenv -func go_getenv(threadIndex C.uintptr_t, name *C.char) (C.bool, *C.zend_string) { - thread := phpThreads[threadIndex] +func go_putenv(name *C.char, nameLen C.int, val *C.char, valLen C.int) C.bool { + goName := C.GoStringN(name, nameLen) - // Get the environment variable value - envValue, exists := getSandboxedEnv(thread)[C.GoString(name)] - if !exists { - // Environment variable does not exist - return false, nil // Return 0 to indicate failure + if val == nil { + // If no "=" is present, unset the environment variable + return C.bool(os.Unsetenv(goName) == nil) } - return true, envValue // Return 1 to indicate success + goVal := C.GoStringN(val, valLen) + return C.bool(os.Setenv(goName, goVal) == nil) } diff --git a/frankenphp.c b/frankenphp.c index 20b36130b6..81fe17a7a8 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -71,9 +71,11 @@ frankenphp_config frankenphp_get_config() { } bool should_filter_var = 0; +HashTable *main_thread_env = NULL; + __thread uintptr_t thread_index; __thread bool is_worker_thread = false; -__thread zval *os_environment = NULL; +__thread HashTable *sandboxed_env = NULL; __thread HashTable *worker_ini_snapshot = NULL; /* Session user handler names (same structure as PS(mod_user_names)). @@ -424,7 +426,7 @@ bool frankenphp_shutdown_dummy_request(void) { } PHPAPI void get_full_env(zval *track_vars_array) { - go_getfullenv(thread_index, track_vars_array); + zend_hash_copy(Z_ARR_P(track_vars_array), main_thread_env, NULL); } /* Adapted from php_request_startup() */ @@ -533,39 +535,72 @@ PHP_FUNCTION(frankenphp_putenv) { RETURN_FALSE; } - if (go_putenv(thread_index, setting, (int)setting_len)) { - RETURN_TRUE; - } else { - RETURN_FALSE; + if (setting_len == 0 || setting[0] == '=') { + zend_argument_value_error(1, "must have a valid syntax"); + RETURN_THROWS(); + } + + if (sandboxed_env == NULL) { + sandboxed_env = zend_array_dup(main_thread_env); + } + + /* cut at null byte to stay consistent with regular putenv */ + char *null_pos = memchr(setting, '\0', setting_len); + if (null_pos != NULL) { + setting_len = null_pos - setting; + } + + /* cut the string at the first '=' */ + char *eq_pos = memchr(setting, '=', setting_len); + bool success = true; + + /* no '=' found, delete the variable */ + if (eq_pos == NULL) { + success = go_putenv(setting, (int)setting_len, NULL, 0); + if (success) { + zend_hash_str_del(sandboxed_env, setting, setting_len); + } + + RETURN_BOOL(success); + } + + size_t name_len = eq_pos - setting; + size_t value_len = + (setting_len > name_len + 1) ? (setting_len - name_len - 1) : 0; + success = go_putenv(setting, (int)name_len, eq_pos + 1, (int)value_len); + if (success) { + zval val = {0}; + ZVAL_STRINGL(&val, eq_pos + 1, value_len); + zend_hash_str_update(sandboxed_env, setting, name_len, &val); } + + RETURN_BOOL(success); } /* }}} */ -/* {{{ Call go's getenv to prevent race conditions */ +/* {{{ Get the env from the sandboxed environment */ PHP_FUNCTION(frankenphp_getenv) { - char *name = NULL; - size_t name_len = 0; + zend_string *name = NULL; bool local_only = 0; ZEND_PARSE_PARAMETERS_START(0, 2) Z_PARAM_OPTIONAL - Z_PARAM_STRING_OR_NULL(name, name_len) + Z_PARAM_STR_OR_NULL(name) Z_PARAM_BOOL(local_only) ZEND_PARSE_PARAMETERS_END(); - if (!name) { - array_init(return_value); - get_full_env(return_value); + HashTable *ht = sandboxed_env ? sandboxed_env : main_thread_env; + if (!name) { + RETURN_ARR(zend_array_dup(ht)); return; } - struct go_getenv_return result = go_getenv(thread_index, name); - - if (result.r0) { - // Return the single environment variable as a string - RETVAL_STR(result.r1); + zval *env_val = zend_hash_find(ht, name); + if (env_val && Z_TYPE_P(env_val) == IS_STRING) { + zend_string *str = Z_STR_P(env_val); + zend_string_addref(str); + RETVAL_STR(str); } else { - // Environment variable does not exist RETVAL_FALSE; } } /* }}} */ @@ -838,14 +873,6 @@ static zend_module_entry frankenphp_module = { TOSTRING(FRANKENPHP_VERSION), STANDARD_MODULE_PROPERTIES}; -static void frankenphp_request_shutdown() { - if (is_worker_thread) { - frankenphp_cleanup_worker_state(); - } - php_request_shutdown((void *)0); - frankenphp_free_request_context(); -} - static int frankenphp_startup(sapi_module_struct *sapi_module) { php_import_environment_variables = get_full_env; @@ -1068,32 +1095,11 @@ static inline void register_server_variable_filtered(const char *key, static void frankenphp_register_variables(zval *track_vars_array) { /* https://www.php.net/manual/en/reserved.variables.server.php */ - /* In CGI mode, we consider the environment to be a part of the server - * variables. + /* In CGI mode, the environment is part of the $_SERVER variables. + * $_SERVER and $_ENV should only contain values from the original + * environment, not values added though putenv */ - - /* in non-worker mode we import the os environment regularly */ - if (!is_worker_thread) { - get_full_env(track_vars_array); - // php_import_environment_variables(track_vars_array); - go_register_variables(thread_index, track_vars_array); - return; - } - - /* In worker mode we cache the os environment */ - if (os_environment == NULL) { - os_environment = malloc(sizeof(zval)); - if (os_environment == NULL) { - php_error(E_ERROR, "Failed to allocate memory for os_environment"); - - return; - } - array_init(os_environment); - get_full_env(os_environment); - // php_import_environment_variables(os_environment); - } - zend_hash_copy(Z_ARR_P(track_vars_array), Z_ARR_P(os_environment), - (copy_ctor_func_t)zval_add_ref); + zend_hash_copy(Z_ARR_P(track_vars_array), main_thread_env, NULL); go_register_variables(thread_index, track_vars_array); } @@ -1103,10 +1109,12 @@ static void frankenphp_log_message(const char *message, int syslog_type_int) { } static char *frankenphp_getenv(const char *name, size_t name_len) { - struct go_getenv_return result = go_getenv(thread_index, (char *)name); + HashTable *ht = sandboxed_env ? sandboxed_env : main_thread_env; - if (result.r0) { - return result.r1->val; + zval *env_val = zend_hash_str_find(ht, name, name_len); + if (env_val && Z_TYPE_P(env_val) == IS_STRING) { + zend_string *str = Z_STR_P(env_val); + return ZSTR_VAL(str); } return NULL; @@ -1242,6 +1250,13 @@ static void *php_main(void *arg) { cfg_get_string("filter.default", &default_filter); should_filter_var = default_filter != NULL; + /* take a snapshot of the environment for sandboxing */ + if (main_thread_env == NULL) { + main_thread_env = pemalloc(sizeof(HashTable), 1); + zend_hash_init(main_thread_env, 8, NULL, NULL, 1); + go_init_os_env(main_thread_env); + } + go_frankenphp_main_thread_is_ready(); /* channel closed, shutdown gracefully */ @@ -1288,7 +1303,8 @@ static int frankenphp_request_startup() { return SUCCESS; } - frankenphp_request_shutdown(); + php_request_shutdown((void *)0); + frankenphp_free_request_context(); return FAILURE; } @@ -1314,16 +1330,20 @@ int frankenphp_execute_script(char *file_name) { zend_catch { status = EG(exit_status); } zend_end_try(); - // free the cached os environment before shutting down the script - if (os_environment != NULL) { - zval_ptr_dtor(os_environment); - free(os_environment); - os_environment = NULL; + zend_destroy_file_handle(&file_handle); + + /* Reset the sandboxed environment */ + if (sandboxed_env != NULL) { + zend_hash_release(sandboxed_env); + sandboxed_env = NULL; } - zend_destroy_file_handle(&file_handle); + if (is_worker_thread) { + frankenphp_cleanup_worker_state(); + } - frankenphp_request_shutdown(); + php_request_shutdown((void *)0); + frankenphp_free_request_context(); return status; } diff --git a/frankenphp_test.go b/frankenphp_test.go index c1120b6c88..7b3e925a0e 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -140,6 +140,12 @@ func TestMain(m *testing.M) { slog.SetDefault(slog.New(slog.DiscardHandler)) } + // setup custom environment var for TestWorkerHasOSEnvironmentVariableInSERVER + if os.Setenv("CUSTOM_OS_ENV_VARIABLE", "custom_env_variable_value") != nil { + fmt.Println("Failed to set environment variable for tests") + os.Exit(1) + } + os.Exit(m.Run()) } @@ -695,11 +701,11 @@ func TestFailingWorker(t *testing.T) { assert.Error(t, err, "should return an immediate error if workers fail on startup") } -func TestEnv(t *testing.T) { - testEnv(t, &testOptions{nbParallelRequests: 1}) +func TestEnv_module(t *testing.T) { + testEnv(t, &testOptions{nbParallelRequests: 1, phpIni: map[string]string{"variables_order": "EGPCS"}}) } -func TestEnvWorker(t *testing.T) { - testEnv(t, &testOptions{nbParallelRequests: 1, workerScript: "env/test-env.php"}) +func TestEnv_worker(t *testing.T) { + testEnv(t, &testOptions{nbParallelRequests: 1, workerScript: "env/test-env.php", phpIni: map[string]string{"variables_order": "EGPCS"}}) } // testEnv cannot be run in parallel due to https://github.com/golang/go/issues/63567 @@ -714,7 +720,7 @@ func testEnv(t *testing.T, opts *testOptions) { stdoutStderr, err := cmd.CombinedOutput() if err != nil { // php is not installed or other issue, use the hardcoded output below: - stdoutStderr = []byte("Set MY_VAR successfully.\nMY_VAR = HelloWorld\nUnset MY_VAR successfully.\nMY_VAR is unset.\nMY_VAR set to empty successfully.\nMY_VAR = \nUnset NON_EXISTING_VAR successfully.\n") + stdoutStderr = []byte("Set MY_VAR successfully.\nMY_VAR = HelloWorld\nMY_VAR not found in $_ENV.\nMY_VAR not found in $_SERVER.\nUnset MY_VAR successfully.\nMY_VAR is unset.\nMY_VAR set to empty successfully.\nMY_VAR = \nUnset NON_EXISTING_VAR successfully.\nInvalid value was not inserted.\n") } assert.Equal(t, string(stdoutStderr), body) diff --git a/internal/extgen/paramparser.go b/internal/extgen/paramparser.go index 3f298904be..74aa2cec56 100644 --- a/internal/extgen/paramparser.go +++ b/internal/extgen/paramparser.go @@ -92,7 +92,7 @@ func (pp *ParameterParser) generateParamParsing(params []phpParameter, requiredC } var builder strings.Builder - builder.WriteString(fmt.Sprintf(" ZEND_PARSE_PARAMETERS_START(%d, %d)", requiredCount, len(params))) + fmt.Fprintf(&builder, " ZEND_PARSE_PARAMETERS_START(%d, %d)", requiredCount, len(params)) optionalStarted := false for _, param := range params { diff --git a/internal/extgen/phpfunc.go b/internal/extgen/phpfunc.go index 1e11172a20..e50c3d8d4c 100644 --- a/internal/extgen/phpfunc.go +++ b/internal/extgen/phpfunc.go @@ -16,7 +16,7 @@ func (pfg *PHPFuncGenerator) generate(fn phpFunction) string { paramInfo := pfg.paramParser.analyzeParameters(fn.Params) funcName := NamespacedName(pfg.namespace, fn.Name) - builder.WriteString(fmt.Sprintf("PHP_FUNCTION(%s)\n{\n", funcName)) + fmt.Fprintf(&builder, "PHP_FUNCTION(%s)\n{\n", funcName) if decl := pfg.paramParser.generateParamDeclarations(fn.Params); decl != "" { builder.WriteString(decl + "\n") diff --git a/phpmainthread.go b/phpmainthread.go index 1ba7dc3d44..d849e79213 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -27,7 +27,6 @@ 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 ( @@ -40,12 +39,11 @@ 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: state.NewThreadState(), - done: make(chan struct{}), - numThreads: numThreads, - maxThreads: numMaxThreads, - phpIni: phpIni, - sandboxedEnv: initializeEnv(), + state: state.NewThreadState(), + done: make(chan struct{}), + numThreads: numThreads, + maxThreads: numMaxThreads, + phpIni: phpIni, } // initialize the first thread diff --git a/phpthread.go b/phpthread.go index 90007cbe17..fdf263717c 100644 --- a/phpthread.go +++ b/phpthread.go @@ -16,13 +16,12 @@ import ( // identified by the index in the phpThreads slice type phpThread struct { runtime.Pinner - threadIndex int - requestChan chan contextHolder - drainChan chan struct{} - handlerMu sync.RWMutex - handler threadHandler - state *state.ThreadState - sandboxedEnv map[string]*C.zend_string + threadIndex int + requestChan chan contextHolder + drainChan chan struct{} + handlerMu sync.RWMutex + handler threadHandler + state *state.ThreadState } // threadHandler defines how the callbacks from the C thread should be handled diff --git a/testdata/env/test-env.php b/testdata/env/test-env.php index b9d374c81c..d3b537ca3f 100644 --- a/testdata/env/test-env.php +++ b/testdata/env/test-env.php @@ -1,28 +1,27 @@