diff --git a/frankenphp.c b/frankenphp.c index 20b36130b..56156b549 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -76,41 +75,6 @@ __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; } @@ -156,11 +120,7 @@ static void frankenphp_reset_super_globals() { zval_ptr_dtor_nogc(files); memset(files, 0, sizeof(*files)); - /* $_SESSION must be explicitly deleted from the symbol table. - * Unlike other superglobals, $_SESSION is stored in EG(symbol_table) - * with a reference to PS(http_session_vars). The session RSHUTDOWN - * only decrements the refcount but doesn't remove it from the symbol - * table, causing data to leak between requests. */ + /* $_SESSION must be explicitly deleted from the symbol table. */ zend_hash_str_del(&EG(symbol_table), "_SESSION", sizeof("_SESSION") - 1); } zend_end_try(); @@ -300,59 +260,6 @@ static void frankenphp_restore_ini(void) { } } -/* 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 = emalloc(sizeof(session_user_handlers)); - - /* 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 */ @@ -361,21 +268,6 @@ static void frankenphp_cleanup_worker_state(void) { 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); \ - } - - FOR_EACH_SESSION_HANDLER(FREE_HANDLER); - -#undef FREE_HANDLER - - efree(worker_session_handlers_snapshot); - worker_session_handlers_snapshot = NULL; - } } /* Adapted from php_request_shutdown */ @@ -412,11 +304,7 @@ 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(); @@ -488,12 +376,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(); diff --git a/frankenphp_test.go b/frankenphp_test.go index c1120b6c8..850580171 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -1207,47 +1207,31 @@ func TestIniLeakBetweenRequests_worker(t *testing.T) { }) } -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") - +func TestSessionHandler_worker(t *testing.T) { + runTest(t, func(handler func(http.ResponseWriter, *http.Request), ts *httptest.Server, i int) { + url := ts.URL + "/worker-with-session-handler.php" + body, _ := testGet(url+"?action=check", handler, t) + assert.Contains(t, body, "no session value", "request without session cookie, should not see session value") + + body, responseWithSessionCookie := testGet(url+"?action=read_session", handler, t) + assert.Contains(t, body, "no session value") + assert.Len(t, responseWithSessionCookie.Header["Set-Cookie"], 1, "Expected exactly one Set-Cookie header") + + requestWithSessionCookie := httptest.NewRequest("GET", url+"?action=put_session", nil) + requestWithSessionCookie.Header["Cookie"] = []string{responseWithSessionCookie.Header["Set-Cookie"][0]} + body, _ = testRequest(requestWithSessionCookie, handler, t) + assert.Contains(t, body, "session value set", "make request with session cookie, should see session value set in previous request") + + body, _ = testGet( url+"?action=read_session", handler, t) + assert.Contains(t, body, "no session value", "make request without session cookie, should not see previous session value") + + body, _ = testGet( url+"?action=check", handler, t) + assert.Contains(t, body, "no session value", "make request without starting a session") + + requestWithSessionCookie = httptest.NewRequest("GET", url+"?action=read_session", nil) + requestWithSessionCookie.Header["Cookie"] = []string{responseWithSessionCookie.Header["Set-Cookie"][0]} + body, _ = testRequest( requestWithSessionCookie, handler, t) + assert.Contains(t, body, "session value exists", "make final request with session cookie, should see previous value") }, &testOptions{ workerScript: "worker-with-session-handler.php", nbWorkers: 1, diff --git a/testdata/worker-with-session-handler.php b/testdata/worker-with-session-handler.php index 218c54769..eebbd9674 100644 --- a/testdata/worker-with-session-handler.php +++ b/testdata/worker-with-session-handler.php @@ -22,6 +22,7 @@ public function read(string $id): string|false public function write(string $id, string $data): bool { + echo "WRITING SESSION: id=$id, data=$data\n"; self::$data[$id] = $data; return true; } @@ -40,59 +41,29 @@ public function gc(int $max_lifetime): int|false // Set the session handler BEFORE the worker loop $handler = new PreLoopSessionHandler(); -session_set_save_handler($handler, true); - -$requestCount = 0; do { - $ok = frankenphp_handle_request(function () use (&$requestCount): void { - $requestCount++; - $output = []; - $output[] = "request=$requestCount"; - + $ok = frankenphp_handle_request(function () use ($handler): void { $action = $_GET['action'] ?? 'check'; switch ($action) { - case 'use_session': - // Try to use the session - should work with pre-loop handler - $error = null; - set_error_handler(function ($errno, $errstr) use (&$error) { - $error = $errstr; - return true; - }); - - try { - session_id('test-preloop-' . $requestCount); - $result = session_start(); - if ($result) { - $_SESSION['test'] = 'value-' . $requestCount; - session_write_close(); - $output[] = "SESSION_OK"; - } else { - $output[] = "SESSION_START_FAILED"; - } - } catch (Throwable $e) { - $output[] = "EXCEPTION:" . $e->getMessage(); - } - - restore_error_handler(); - - if ($error) { - $output[] = "ERROR:" . $error; - } + case 'put_session': + session_set_save_handler($handler, true); + session_start(); + $_SESSION['test'] = 'session value exists'; + echo 'session value set'; + break; + case 'read_session': + session_set_save_handler($handler, true); + session_start(); + echo 'session id:' . session_id(); + echo $_SESSION['test'] ?? 'no session value'; 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 $_SESSION['test'] ?? 'no session value'; + break; } - - echo implode("\n", $output); }); } while ($ok);