diff --git a/caddy/caddy_test.go b/caddy/caddy_test.go index 7cf69bfa5e..96ea7eb50c 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/env.go b/env.go index 3ac9a3adf6..31963ebd3e 100644 --- a/env.go +++ b/env.go @@ -11,106 +11,29 @@ 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 { +//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) -} + zkey := C.frankenphp_init_persistent_string(toUnsafeChar(key), C.size_t(len(key))) + zvalStr := C.frankenphp_init_persistent_string(toUnsafeChar(val), C.size_t(len(val))) -// 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 + var zval C.zval + *(*uint32)(unsafe.Pointer(&zval.u1)) = C.IS_INTERNED_STRING_EX + *(**C.zend_string)(unsafe.Pointer(&zval.value)) = zvalStr + C.zend_hash_update(mainThreadEnv, zkey, &zval) } } //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 04782a9b65..6a3fa36075 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -70,9 +70,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; void frankenphp_update_local_thread_context(bool is_worker) { is_worker_thread = is_worker; @@ -206,7 +208,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() */ @@ -303,39 +305,64 @@ PHP_FUNCTION(frankenphp_putenv) { RETURN_FALSE; } - if (go_putenv(thread_index, setting, (int)setting_len)) { - RETURN_TRUE; + 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 the string at the first '=' + char *eq_pos = memchr(setting, '=', setting_len); + bool put_env_success = true; + if (eq_pos != NULL) { + size_t name_len = eq_pos - setting; + size_t value_len = + (setting_len > name_len + 1) ? (setting_len - name_len - 1) : 0; + put_env_success = + go_putenv(setting, (int)name_len, eq_pos + 1, (int)value_len); + if (put_env_success) { + zval val = {0}; + ZVAL_STRINGL(&val, eq_pos + 1, value_len); + zend_hash_str_update(sandboxed_env, setting, name_len, &val); + } } else { - RETURN_FALSE; + // no '=' found, delete the variable + put_env_success = go_putenv(setting, (int)setting_len, NULL, 0); + if (put_env_success) { + zend_hash_str_del(sandboxed_env, setting, setting_len); + } } + + RETURN_BOOL(put_env_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; } } /* }}} */ @@ -585,11 +612,6 @@ static zend_module_entry frankenphp_module = { TOSTRING(FRANKENPHP_VERSION), STANDARD_MODULE_PROPERTIES}; -static void frankenphp_request_shutdown() { - frankenphp_free_request_context(); - php_request_shutdown((void *)0); -} - static int frankenphp_startup(sapi_module_struct *sapi_module) { php_import_environment_variables = get_full_env; @@ -812,32 +834,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); } @@ -847,10 +848,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; @@ -986,6 +989,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 = malloc(sizeof(HashTable)); + 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 */ @@ -1059,16 +1069,16 @@ 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); - frankenphp_request_shutdown(); + /* Reset values the sandboxed environment */ + if (sandboxed_env != NULL) { + zend_hash_release(sandboxed_env); + sandboxed_env = NULL; + } + + frankenphp_free_request_context(); + php_request_shutdown((void *)0); return status; } diff --git a/frankenphp_test.go b/frankenphp_test.go index 427b731f19..07b9590bb1 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -147,6 +147,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()) } @@ -645,7 +651,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 $_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/phpmainthread.go b/phpmainthread.go index cecadc1653..af44aaa3d2 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 1726cf9d18..7fc5a515c3 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.Mutex - handler threadHandler - state *state.ThreadState - sandboxedEnv map[string]*C.zend_string + threadIndex int + requestChan chan contextHolder + drainChan chan struct{} + handlerMu sync.Mutex + handler threadHandler + state *state.ThreadState } // interface that 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..002a45df02 100644 --- a/testdata/env/test-env.php +++ b/testdata/env/test-env.php @@ -13,6 +13,14 @@ echo "Failed to set MY_VAR.\n"; } + // verify putenv does not affect $_SERVER + $result = $_SERVER[$var] ?? null; + if ($result !== null) { + echo "MY_VAR is in \$_SERVER (not expected)\n"; + } else { + echo "MY_VAR not found in \$_SERVER.\n"; + } + // Unsetting the environment variable $result = putenv($var); if ($result) { @@ -48,5 +56,13 @@ echo "Failed to unset NON_EXISTING_VAR.\n"; } + // Inserting an invalid variable should fail (null byte in key) + putenv("INVALID\x0_VAR=value"); + if (getenv("INVALID\x0_VAR")) { + echo "Invalid value was inserted (unexpected).\n"; + } else { + echo "Invalid value was not inserted.\n"; + } + getenv(); }; diff --git a/threadregular.go b/threadregular.go index ad44c57170..938d98350c 100644 --- a/threadregular.go +++ b/threadregular.go @@ -76,9 +76,6 @@ func (handler *regularThread) name() string { } func (handler *regularThread) waitForRequest() string { - // clear any previously sandboxed env - clearSandboxedEnv(handler.thread) - handler.state.MarkAsWaiting(true) var ch contextHolder diff --git a/threadworker.go b/threadworker.go index ae7e4545f2..84af4d430a 100644 --- a/threadworker.go +++ b/threadworker.go @@ -120,7 +120,6 @@ func setupWorkerScript(handler *workerThread, worker *worker) { handler.dummyFrankenPHPContext = fc handler.dummyContext = ctx handler.isBootingScript = true - clearSandboxedEnv(handler.thread) if globalLogger.Enabled(ctx, slog.LevelDebug) { globalLogger.LogAttrs(ctx, slog.LevelDebug, "starting", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex)) diff --git a/worker_test.go b/worker_test.go index c6b4245d47..65e019538e 100644 --- a/worker_test.go +++ b/worker_test.go @@ -8,13 +8,10 @@ import ( "net/http" "net/http/httptest" "net/url" - "os" "strconv" "strings" "testing" - "github.com/stretchr/testify/require" - "github.com/dunglas/frankenphp" "github.com/stretchr/testify/assert" "go.uber.org/zap/exp/zapslog" @@ -150,8 +147,6 @@ func ExampleServeHTTP_workers() { } func TestWorkerHasOSEnvironmentVariableInSERVER(t *testing.T) { - require.NoError(t, os.Setenv("CUSTOM_OS_ENV_VARIABLE", "custom_env_variable_value")) - runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) { req := httptest.NewRequest("GET", "http://example.com/worker.php", nil) w := httptest.NewRecorder()