diff --git a/frankenphp.c b/frankenphp.c index 67ffdf28b..c943c2218 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -4,9 +4,11 @@ #include #include #include -#include #include #include +#ifdef HAVE_PHP_SESSION +#include +#endif #include #include #include @@ -41,7 +43,7 @@ ZEND_TSRMLS_CACHE_DEFINE() * * @see https://github.com/DataDog/dd-trace-php/pull/3169 for an example */ -static const char *MODULES_TO_RELOAD[] = {"filter", "session", NULL}; +static const char *MODULES_TO_RELOAD[] = {"filter", NULL}; frankenphp_version frankenphp_get_version() { return (frankenphp_version){ @@ -76,42 +78,6 @@ bool original_user_abort_setting = 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 - -#define FOR_EACH_SESSION_HANDLER(op) \ - op(ps_open); \ - op(ps_close); \ - op(ps_read); \ - op(ps_write); \ - op(ps_destroy); \ - op(ps_gc); \ - op(ps_create_sid); \ - op(ps_validate_sid); \ - op(ps_update_timestamp) - -__thread session_user_handlers *worker_session_handlers_snapshot = NULL; void frankenphp_update_local_thread_context(bool is_worker) { is_worker_thread = is_worker; @@ -223,165 +189,52 @@ static void frankenphp_release_temporary_streams() { ZEND_HASH_FOREACH_END(); } -/* Destructor for INI snapshot hash table entries */ -static void frankenphp_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 */ - } - - if (EG(modified_ini_directives) == NULL) { - /* Allocate empty table to mark as snapshotted */ - ALLOC_HASHTABLE(worker_ini_snapshot); - zend_hash_init(worker_ini_snapshot, 0, NULL, frankenphp_ini_snapshot_dtor, - 0); - return; - } - - uint32_t num_modified = zend_hash_num_elements(EG(modified_ini_directives)); - ALLOC_HASHTABLE(worker_ini_snapshot); - zend_hash_init(worker_ini_snapshot, num_modified, NULL, - frankenphp_ini_snapshot_dtor, 0); - - 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; +#ifdef HAVE_PHP_SESSION +/* Reset session state between worker requests, preserving user handlers. + * Based on php_rshutdown_session_globals() + php_rinit_session_globals(). */ +static void frankenphp_reset_session_state(void) { + if (PS(session_status) == php_session_active) { + php_session_flush(1); } - 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). */ - uint32_t max_entries = zend_hash_num_elements(EG(modified_ini_directives)); - zend_string **entries_to_restore = - max_entries ? emalloc(max_entries * sizeof(zend_string *)) : NULL; - size_t restore_count = 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 */ - 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 */ + if (!Z_ISUNDEF(PS(http_session_vars))) { + zval_ptr_dtor(&PS(http_session_vars)); + ZVAL_UNDEF(&PS(http_session_vars)); } - 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 */ + if (PS(mod_data) || PS(mod_user_implemented)) { + zend_try { PS(mod)->s_close(&PS(mod_data)); } + zend_end_try(); } - /* Check if session module is loaded */ - if (zend_hash_str_find_ptr(&module_registry, "session", - sizeof("session") - 1) == NULL) { - return; /* Session module not available */ + if (PS(id)) { + zend_string_release_ex(PS(id), 0); + PS(id) = NULL; } - /* Check if user session handlers are defined */ - if (Z_ISUNDEF(PS_MOD_USER_NAMES(ps_open))) { - return; /* No user handlers to snapshot */ + if (PS(session_vars)) { + zend_string_release_ex(PS(session_vars), 0); + PS(session_vars) = NULL; } - worker_session_handlers_snapshot = emalloc(sizeof(session_user_handlers)); + /* PS(mod_user_class_name) and PS(mod_user_names) are preserved */ - /* Copy each handler zval with incremented reference count */ -#define SNAPSHOT_HANDLER(h) \ - if (!Z_ISUNDEF(PS_MOD_USER_NAMES(h))) { \ - ZVAL_COPY(&worker_session_handlers_snapshot->h, &PS_MOD_USER_NAMES(h)); \ - } else { \ - ZVAL_UNDEF(&worker_session_handlers_snapshot->h); \ - } - - FOR_EACH_SESSION_HANDLER(SNAPSHOT_HANDLER); - -#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. - * Session RSHUTDOWN already freed the handlers via zval_ptr_dtor and set - * them to UNDEF, so we don't need to destroy them again. We simply copy - * from the snapshot (which holds its own reference). */ -#define RESTORE_HANDLER(h) \ - if (!Z_ISUNDEF(worker_session_handlers_snapshot->h)) { \ - ZVAL_COPY(&PS_MOD_USER_NAMES(h), &worker_session_handlers_snapshot->h); \ - } - - FOR_EACH_SESSION_HANDLER(RESTORE_HANDLER); - -#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(h) \ - if (!Z_ISUNDEF(worker_session_handlers_snapshot->h)) { \ - zval_ptr_dtor(&worker_session_handlers_snapshot->h); \ +#if PHP_VERSION_ID >= 80300 + if (PS(session_started_filename)) { + zend_string_release(PS(session_started_filename)); + PS(session_started_filename) = NULL; + PS(session_started_lineno) = 0; } +#endif - FOR_EACH_SESSION_HANDLER(FREE_HANDLER); - -#undef FREE_HANDLER - - efree(worker_session_handlers_snapshot); - worker_session_handlers_snapshot = NULL; - } + PS(session_status) = php_session_none; + PS(in_save_handler) = 0; + PS(set_handler) = 0; + PS(mod_data) = NULL; + PS(mod_user_is_open) = 0; + PS(define_sid) = 1; } +#endif /* Adapted from php_request_shutdown */ static void frankenphp_worker_request_shutdown() { @@ -398,6 +251,10 @@ static void frankenphp_worker_request_shutdown() { } } +#ifdef HAVE_PHP_SESSION + frankenphp_reset_session_state(); +#endif + /* Shutdown output layer (send the set HTTP headers, cleanup output handlers, * etc.) */ zend_try { php_output_deactivate(); } @@ -417,12 +274,6 @@ 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; @@ -478,12 +329,6 @@ 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++) { @@ -493,12 +338,6 @@ 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(); @@ -844,9 +683,6 @@ static zend_module_entry frankenphp_module = { 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(); } diff --git a/frankenphp_test.go b/frankenphp_test.go index c1120b6c8..eb39ea869 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -1143,8 +1143,8 @@ func TestSessionHandlerReset_worker(t *testing.T) { 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 + // The user handler from request 1 is preserved (mod_user_names persist), + // so session_start() should work without crashing. resp2, err := http.Get(ts.URL + "/session-handler.php?action=start_without_handler") assert.NoError(t, err) body2, _ := io.ReadAll(resp2.Body) @@ -1152,13 +1152,9 @@ func TestSessionHandlerReset_worker(t *testing.T) { 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 + // session_start() should succeed (handlers are preserved) assert.Contains(t, body2Str, "SESSION_START_RESULT=true", - "session_start() should succeed after INI reset.\nResponse: %s", body2Str) + "session_start() should succeed.\nResponse: %s", body2Str) // No errors or exceptions should occur assert.NotContains(t, body2Str, "ERROR:", @@ -1174,39 +1170,6 @@ func TestSessionHandlerReset_worker(t *testing.T) { }) } -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 @@ -1256,58 +1219,6 @@ func TestSessionHandlerPreLoopPreserved_worker(t *testing.T) { }) } -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, - }) -} - func TestSessionNoLeakBetweenRequests_worker(t *testing.T) { runTest(t, func(_ func(http.ResponseWriter, *http.Request), ts *httptest.Server, i int) { // Client A: Set a secret value in session diff --git a/testdata/ini-leak.php b/testdata/ini-leak.php deleted file mode 100644 index 286f56aa9..000000000 --- a/testdata/ini-leak.php +++ /dev/null @@ -1,67 +0,0 @@ -