diff --git a/frankenphp.c b/frankenphp.c index fd487edb8e..ebe7bc9cb0 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -73,6 +74,31 @@ bool should_filter_var = 0; __thread uintptr_t thread_index; __thread bool is_worker_thread = false; __thread zval *os_environment = NULL; +__thread HashTable *worker_ini_snapshot = NULL; + +/* Session user handler names (same structure as PS(mod_user_names)). + * In PHP 8.2, mod_user_names is a union with .name.ps_* access. + * In PHP 8.3+, mod_user_names is a direct struct with .ps_* access. */ +typedef struct { + zval ps_open; + zval ps_close; + zval ps_read; + zval ps_write; + zval ps_destroy; + zval ps_gc; + zval ps_create_sid; + zval ps_validate_sid; + zval ps_update_timestamp; +} session_user_handlers; + +/* Macro to access PS(mod_user_names) handlers across PHP versions */ +#if PHP_VERSION_ID >= 80300 +#define PS_MOD_USER_NAMES(handler) PS(mod_user_names).handler +#else +#define PS_MOD_USER_NAMES(handler) PS(mod_user_names).name.handler +#endif + +__thread session_user_handlers *worker_session_handlers_snapshot = NULL; void frankenphp_update_local_thread_context(bool is_worker) { is_worker_thread = is_worker; @@ -166,6 +192,193 @@ static void frankenphp_release_temporary_streams() { ZEND_HASH_FOREACH_END(); } +/* Destructor for INI snapshot hash table entries */ +static void ini_snapshot_dtor(zval *zv) { + zend_string_release((zend_string *)Z_PTR_P(zv)); +} + +/* Save the current state of modified INI entries. + * This captures INI values set by the framework before the worker loop. */ +static void frankenphp_snapshot_ini(void) { + if (worker_ini_snapshot != NULL) { + return; /* Already snapshotted */ + } + + ALLOC_HASHTABLE(worker_ini_snapshot); + zend_hash_init(worker_ini_snapshot, 8, NULL, ini_snapshot_dtor, 0); + + if (EG(modified_ini_directives) == NULL) { + return; /* No modifications to snapshot */ + } + + zend_ini_entry *ini_entry; + ZEND_HASH_FOREACH_PTR(EG(modified_ini_directives), ini_entry) { + if (ini_entry->value) { + zend_hash_add_ptr(worker_ini_snapshot, ini_entry->name, + zend_string_copy(ini_entry->value)); + } + } + ZEND_HASH_FOREACH_END(); +} + +/* Restore INI values to the state captured by frankenphp_snapshot_ini(). + * - Entries in snapshot with changed values: restore to snapshot value + * - Entries not in snapshot: restore to startup default */ +static void frankenphp_restore_ini(void) { + if (worker_ini_snapshot == NULL || EG(modified_ini_directives) == NULL) { + return; + } + + zend_ini_entry *ini_entry; + zend_string *snapshot_value; + zend_string *entry_name; + + /* Collect entries to restore to default in a separate array. + * We cannot call zend_restore_ini_entry() during iteration because + * it calls zend_hash_del() on EG(modified_ini_directives). */ + zend_string **entries_to_restore = NULL; + size_t restore_count = 0; + size_t restore_capacity = 0; + + ZEND_HASH_FOREACH_STR_KEY_PTR(EG(modified_ini_directives), entry_name, + ini_entry) { + snapshot_value = zend_hash_find_ptr(worker_ini_snapshot, entry_name); + + if (snapshot_value == NULL) { + /* Entry was not in snapshot: collect for restore to startup default */ + if (restore_count >= restore_capacity) { + restore_capacity = restore_capacity ? restore_capacity * 2 : 8; + entries_to_restore = erealloc(entries_to_restore, + restore_capacity * sizeof(zend_string *)); + } + entries_to_restore[restore_count++] = zend_string_copy(entry_name); + } else if (!zend_string_equals(ini_entry->value, snapshot_value)) { + /* Entry was in snapshot but value changed: restore to snapshot value. + * zend_alter_ini_entry() does not delete from modified_ini_directives. */ + zend_alter_ini_entry(entry_name, snapshot_value, PHP_INI_USER, + PHP_INI_STAGE_RUNTIME); + } + /* else: Entry in snapshot with same value, nothing to do */ + } + ZEND_HASH_FOREACH_END(); + + /* Now restore entries to default outside of iteration */ + for (size_t i = 0; i < restore_count; i++) { + zend_restore_ini_entry(entries_to_restore[i], PHP_INI_STAGE_RUNTIME); + zend_string_release(entries_to_restore[i]); + } + if (entries_to_restore) { + efree(entries_to_restore); + } +} + +/* Save session user handlers set before the worker loop. + * This allows frameworks to define custom session handlers that persist. */ +static void frankenphp_snapshot_session_handlers(void) { + if (worker_session_handlers_snapshot != NULL) { + return; /* Already snapshotted */ + } + + /* Check if session module is loaded */ + if (zend_hash_str_find_ptr(&module_registry, "session", + sizeof("session") - 1) == NULL) { + return; /* Session module not available */ + } + + /* Check if user session handlers are defined */ + if (Z_ISUNDEF(PS_MOD_USER_NAMES(ps_open))) { + return; /* No user handlers to snapshot */ + } + + worker_session_handlers_snapshot = malloc(sizeof(session_user_handlers)); + if (worker_session_handlers_snapshot == NULL) { + return; /* Memory allocation failed */ + } + + /* Copy each handler zval with incremented reference count */ +#define SNAPSHOT_HANDLER(handler) \ + if (!Z_ISUNDEF(PS_MOD_USER_NAMES(handler))) { \ + ZVAL_COPY(&worker_session_handlers_snapshot->handler, \ + &PS_MOD_USER_NAMES(handler)); \ + } else { \ + ZVAL_UNDEF(&worker_session_handlers_snapshot->handler); \ + } + + SNAPSHOT_HANDLER(ps_open); + SNAPSHOT_HANDLER(ps_close); + SNAPSHOT_HANDLER(ps_read); + SNAPSHOT_HANDLER(ps_write); + SNAPSHOT_HANDLER(ps_destroy); + SNAPSHOT_HANDLER(ps_gc); + SNAPSHOT_HANDLER(ps_create_sid); + SNAPSHOT_HANDLER(ps_validate_sid); + SNAPSHOT_HANDLER(ps_update_timestamp); + +#undef SNAPSHOT_HANDLER +} + +/* Restore session user handlers from snapshot after RSHUTDOWN freed them. */ +static void frankenphp_restore_session_handlers(void) { + if (worker_session_handlers_snapshot == NULL) { + return; + } + + /* Restore each handler zval */ +#define RESTORE_HANDLER(handler) \ + if (!Z_ISUNDEF(worker_session_handlers_snapshot->handler)) { \ + if (!Z_ISUNDEF(PS_MOD_USER_NAMES(handler))) { \ + zval_ptr_dtor(&PS_MOD_USER_NAMES(handler)); \ + } \ + ZVAL_COPY(&PS_MOD_USER_NAMES(handler), \ + &worker_session_handlers_snapshot->handler); \ + } + + RESTORE_HANDLER(ps_open); + RESTORE_HANDLER(ps_close); + RESTORE_HANDLER(ps_read); + RESTORE_HANDLER(ps_write); + RESTORE_HANDLER(ps_destroy); + RESTORE_HANDLER(ps_gc); + RESTORE_HANDLER(ps_create_sid); + RESTORE_HANDLER(ps_validate_sid); + RESTORE_HANDLER(ps_update_timestamp); + +#undef RESTORE_HANDLER +} + +/* Free worker state when the worker script terminates. */ +static void frankenphp_cleanup_worker_state(void) { + /* Free INI snapshot */ + if (worker_ini_snapshot != NULL) { + zend_hash_destroy(worker_ini_snapshot); + FREE_HASHTABLE(worker_ini_snapshot); + worker_ini_snapshot = NULL; + } + + /* Free session handlers snapshot */ + if (worker_session_handlers_snapshot != NULL) { +#define FREE_HANDLER(handler) \ + if (!Z_ISUNDEF(worker_session_handlers_snapshot->handler)) { \ + zval_ptr_dtor(&worker_session_handlers_snapshot->handler); \ + } + + FREE_HANDLER(ps_open); + FREE_HANDLER(ps_close); + FREE_HANDLER(ps_read); + FREE_HANDLER(ps_write); + FREE_HANDLER(ps_destroy); + FREE_HANDLER(ps_gc); + FREE_HANDLER(ps_create_sid); + FREE_HANDLER(ps_validate_sid); + FREE_HANDLER(ps_update_timestamp); + +#undef FREE_HANDLER + + free(worker_session_handlers_snapshot); + worker_session_handlers_snapshot = NULL; + } +} + /* Adapted from php_request_shutdown */ static void frankenphp_worker_request_shutdown() { /* Flush all output buffers */ @@ -200,6 +413,12 @@ bool frankenphp_shutdown_dummy_request(void) { return false; } + /* Snapshot INI and session handlers BEFORE shutdown. + * The framework has set these up before the worker loop, and we want + * to preserve them. Session RSHUTDOWN will free the handlers. */ + frankenphp_snapshot_ini(); + frankenphp_snapshot_session_handlers(); + frankenphp_worker_request_shutdown(); return true; @@ -255,6 +474,12 @@ static int frankenphp_worker_request_startup() { frankenphp_reset_super_globals(); + /* Restore INI values changed during the previous request back to their + * snapshot state (captured in frankenphp_shutdown_dummy_request). + * This ensures framework settings persist while request-level changes + * are reset. */ + frankenphp_restore_ini(); + const char **module_name; zend_module_entry *module; for (module_name = MODULES_TO_RELOAD; *module_name; module_name++) { @@ -264,6 +489,12 @@ static int frankenphp_worker_request_startup() { module->request_startup_func(module->type, module->module_number); } } + + /* Restore session handlers AFTER session RINIT. + * Session RSHUTDOWN frees mod_user_names callbacks, so we must restore + * them before user code runs. This must happen after RINIT because + * session RINIT may reset some state. */ + frankenphp_restore_session_handlers(); } zend_catch { retval = FAILURE; } zend_end_try(); @@ -610,6 +841,9 @@ static zend_module_entry frankenphp_module = { static void frankenphp_request_shutdown() { frankenphp_free_request_context(); + if (is_worker_thread) { + frankenphp_cleanup_worker_state(); + } php_request_shutdown((void *)0); } diff --git a/frankenphp_test.go b/frankenphp_test.go index 8c6f3c90da..8cf9fc8021 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -1078,3 +1078,181 @@ func FuzzRequest(f *testing.F) { }, &testOptions{workerScript: "request-headers.php"}) }) } + +func TestSessionHandlerReset_worker(t *testing.T) { + runTest(t, func(_ func(http.ResponseWriter, *http.Request), ts *httptest.Server, i int) { + // Request 1: Set a custom session handler and start session + resp1, err := http.Get(ts.URL + "/session-handler.php?action=set_handler_and_start&value=test1") + assert.NoError(t, err) + body1, _ := io.ReadAll(resp1.Body) + _ = resp1.Body.Close() + + body1Str := string(body1) + assert.Contains(t, body1Str, "HANDLER_SET_AND_STARTED") + assert.Contains(t, body1Str, "session.save_handler=user") + + // Request 2: Start session without setting a custom handler + // After the fix: session.save_handler should be reset to "files" + // and session_start() should work normally + resp2, err := http.Get(ts.URL + "/session-handler.php?action=start_without_handler") + assert.NoError(t, err) + body2, _ := io.ReadAll(resp2.Body) + _ = resp2.Body.Close() + + body2Str := string(body2) + + // session.save_handler should be reset to "files" (default) + assert.Contains(t, body2Str, "save_handler_before=files", + "session.save_handler INI should be reset to 'files' between requests.\nResponse: %s", body2Str) + + // session_start() should succeed + assert.Contains(t, body2Str, "SESSION_START_RESULT=true", + "session_start() should succeed after INI reset.\nResponse: %s", body2Str) + + // No errors or exceptions should occur + assert.NotContains(t, body2Str, "ERROR:", + "No errors expected.\nResponse: %s", body2Str) + assert.NotContains(t, body2Str, "EXCEPTION:", + "No exceptions expected.\nResponse: %s", body2Str) + + }, &testOptions{ + workerScript: "session-handler.php", + nbWorkers: 1, + nbParallelRequests: 1, + realServer: true, + }) +} + +func TestIniLeakBetweenRequests_worker(t *testing.T) { + runTest(t, func(_ func(http.ResponseWriter, *http.Request), ts *httptest.Server, i int) { + // Request 1: Change INI values + resp1, err := http.Get(ts.URL + "/ini-leak.php?action=change_ini") + assert.NoError(t, err) + body1, _ := io.ReadAll(resp1.Body) + _ = resp1.Body.Close() + + assert.Contains(t, string(body1), "INI_CHANGED") + + // Request 2: Check if INI values leaked from request 1 + resp2, err := http.Get(ts.URL + "/ini-leak.php?action=check_ini") + assert.NoError(t, err) + body2, _ := io.ReadAll(resp2.Body) + _ = resp2.Body.Close() + + body2Str := string(body2) + t.Logf("Response: %s", body2Str) + + // If INI values leak, this test will fail + assert.Contains(t, body2Str, "NO_LEAKS", + "INI values should not leak between requests.\nResponse: %s", body2Str) + assert.NotContains(t, body2Str, "LEAKS_DETECTED", + "INI leaks detected.\nResponse: %s", body2Str) + + }, &testOptions{ + workerScript: "ini-leak.php", + nbWorkers: 1, + nbParallelRequests: 1, + realServer: true, + }) +} + +func TestSessionHandlerPreLoopPreserved_worker(t *testing.T) { + runTest(t, func(_ func(http.ResponseWriter, *http.Request), ts *httptest.Server, i int) { + // Request 1: Check that the pre-loop session handler is preserved + resp1, err := http.Get(ts.URL + "/worker-with-session-handler.php?action=check") + assert.NoError(t, err) + body1, _ := io.ReadAll(resp1.Body) + _ = resp1.Body.Close() + + body1Str := string(body1) + t.Logf("Request 1 response: %s", body1Str) + assert.Contains(t, body1Str, "HANDLER_PRESERVED", + "Session handler set before loop should be preserved") + assert.Contains(t, body1Str, "save_handler=user", + "session.save_handler should remain 'user'") + + // Request 2: Use the session - should work with pre-loop handler + resp2, err := http.Get(ts.URL + "/worker-with-session-handler.php?action=use_session") + assert.NoError(t, err) + body2, _ := io.ReadAll(resp2.Body) + _ = resp2.Body.Close() + + body2Str := string(body2) + t.Logf("Request 2 response: %s", body2Str) + assert.Contains(t, body2Str, "SESSION_OK", + "Session should work with pre-loop handler.\nResponse: %s", body2Str) + assert.NotContains(t, body2Str, "ERROR:", + "No errors expected.\nResponse: %s", body2Str) + assert.NotContains(t, body2Str, "EXCEPTION:", + "No exceptions expected.\nResponse: %s", body2Str) + + // Request 3: Check handler is still preserved after using session + resp3, err := http.Get(ts.URL + "/worker-with-session-handler.php?action=check") + assert.NoError(t, err) + body3, _ := io.ReadAll(resp3.Body) + _ = resp3.Body.Close() + + body3Str := string(body3) + t.Logf("Request 3 response: %s", body3Str) + assert.Contains(t, body3Str, "HANDLER_PRESERVED", + "Session handler should still be preserved after use") + + }, &testOptions{ + workerScript: "worker-with-session-handler.php", + nbWorkers: 1, + nbParallelRequests: 1, + realServer: true, + }) +} + +func TestIniPreLoopPreserved_worker(t *testing.T) { + runTest(t, func(_ func(http.ResponseWriter, *http.Request), ts *httptest.Server, i int) { + // Request 1: Check that pre-loop INI values are present + resp1, err := http.Get(ts.URL + "/worker-with-ini.php?action=check") + assert.NoError(t, err) + body1, _ := io.ReadAll(resp1.Body) + _ = resp1.Body.Close() + + body1Str := string(body1) + t.Logf("Request 1 response: %s", body1Str) + assert.Contains(t, body1Str, "precision=8", + "Pre-loop precision should be 8") + assert.Contains(t, body1Str, "display_errors=0", + "Pre-loop display_errors should be 0") + assert.Contains(t, body1Str, "PRELOOP_INI_PRESERVED", + "Pre-loop INI values should be preserved") + + // Request 2: Change INI values during request + resp2, err := http.Get(ts.URL + "/worker-with-ini.php?action=change_ini") + assert.NoError(t, err) + body2, _ := io.ReadAll(resp2.Body) + _ = resp2.Body.Close() + + body2Str := string(body2) + t.Logf("Request 2 response: %s", body2Str) + assert.Contains(t, body2Str, "INI_CHANGED") + assert.Contains(t, body2Str, "precision=5", + "INI should be changed during request") + + // Request 3: Check that pre-loop INI values are restored + resp3, err := http.Get(ts.URL + "/worker-with-ini.php?action=check") + assert.NoError(t, err) + body3, _ := io.ReadAll(resp3.Body) + _ = resp3.Body.Close() + + body3Str := string(body3) + t.Logf("Request 3 response: %s", body3Str) + assert.Contains(t, body3Str, "precision=8", + "Pre-loop precision should be restored to 8.\nResponse: %s", body3Str) + assert.Contains(t, body3Str, "display_errors=0", + "Pre-loop display_errors should be restored to 0.\nResponse: %s", body3Str) + assert.Contains(t, body3Str, "PRELOOP_INI_PRESERVED", + "Pre-loop INI values should be restored after request changes.\nResponse: %s", body3Str) + + }, &testOptions{ + workerScript: "worker-with-ini.php", + nbWorkers: 1, + nbParallelRequests: 1, + realServer: true, + }) +} diff --git a/testdata/ini-leak.php b/testdata/ini-leak.php new file mode 100644 index 0000000000..286f56aa9d --- /dev/null +++ b/testdata/ini-leak.php @@ -0,0 +1,67 @@ +getMessage(); + } + + restore_error_handler(); + + // Now output everything + $output[] = "save_handler_before=" . $saveHandlerBefore; + $output[] = "SESSION_START_RESULT=" . ($result ? "true" : "false"); + if ($error) { + $output[] = "ERROR:" . $error; + } + if ($exception) { + $output[] = "EXCEPTION:" . $exception; + } + break; + + case 'just_start': + // Simple session start without any custom handler + // This should always work + session_id('test-session-id-3'); + session_start(); + $_SESSION['test'] = 'value'; + session_write_close(); + $output[] = "SESSION_STARTED_OK"; + break; + + default: + $output[] = "UNKNOWN_ACTION"; + } + + echo implode("\n", $output); +}; diff --git a/testdata/worker-with-ini.php b/testdata/worker-with-ini.php new file mode 100644 index 0000000000..31e52395dc --- /dev/null +++ b/testdata/worker-with-ini.php @@ -0,0 +1,63 @@ +getMessage(); + } + + restore_error_handler(); + + if ($error) { + $output[] = "ERROR:" . $error; + } + break; + + case 'check': + default: + $saveHandler = ini_get('session.save_handler'); + $output[] = "save_handler=$saveHandler"; + if ($saveHandler === 'user') { + $output[] = "HANDLER_PRESERVED"; + } else { + $output[] = "HANDLER_LOST"; + } + } + + echo implode("\n", $output); + }); +} while ($ok);