From 60fe0966da7b7bc8e7b22ccbeef974fa7b5a44a0 Mon Sep 17 00:00:00 2001 From: Xavier Leune Date: Fri, 23 Jan 2026 11:26:39 +0100 Subject: [PATCH 1/4] fix(worker): reset ini and session if changed during worker --- frankenphp.c | 225 +++++++++++++++++++++++ frankenphp_test.go | 178 ++++++++++++++++++ testdata/ini-leak.php | 67 +++++++ testdata/session-handler.php | 115 ++++++++++++ testdata/worker-with-ini.php | 63 +++++++ testdata/worker-with-session-handler.php | 98 ++++++++++ 6 files changed, 746 insertions(+) create mode 100644 testdata/ini-leak.php create mode 100644 testdata/session-handler.php create mode 100644 testdata/worker-with-ini.php create mode 100644 testdata/worker-with-session-handler.php diff --git a/frankenphp.c b/frankenphp.c index d98b1d7b8..1bd1240d7 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -73,6 +74,22 @@ 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)) */ +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; + +__thread session_user_handlers *worker_session_handlers_snapshot = NULL; void frankenphp_update_local_thread_context(bool is_worker) { is_worker_thread = is_worker; @@ -174,6 +191,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(name) \ + if (!Z_ISUNDEF(PS(mod_user_names).name)) { \ + ZVAL_COPY(&worker_session_handlers_snapshot->name, \ + &PS(mod_user_names).name); \ + } else { \ + ZVAL_UNDEF(&worker_session_handlers_snapshot->name); \ + } + + 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(name) \ + if (!Z_ISUNDEF(worker_session_handlers_snapshot->name)) { \ + if (!Z_ISUNDEF(PS(mod_user_names).name)) { \ + zval_ptr_dtor(&PS(mod_user_names).name); \ + } \ + ZVAL_COPY(&PS(mod_user_names).name, \ + &worker_session_handlers_snapshot->name); \ + } + + 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(name) \ + if (!Z_ISUNDEF(worker_session_handlers_snapshot->name)) { \ + zval_ptr_dtor(&worker_session_handlers_snapshot->name); \ + } + + 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 */ @@ -208,6 +412,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; @@ -263,6 +473,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++) { @@ -272,6 +488,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(); @@ -617,6 +839,9 @@ 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 937853f67..d3e96e1b5 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -1128,3 +1128,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 000000000..286f56aa9 --- /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 000000000..31e52395d --- /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); From cc08195568c0e1c7e064aef967252e3efd491326 Mon Sep 17 00:00:00 2001 From: Xavier Leune Date: Fri, 23 Jan 2026 11:45:00 +0100 Subject: [PATCH 2/4] fix(worker): reset ini and session: support PHP8.2 --- frankenphp.c | 45 +++++++++++++++++++++++++++------------------ frankenphp_test.go | 22 +++++++++++----------- 2 files changed, 38 insertions(+), 29 deletions(-) diff --git a/frankenphp.c b/frankenphp.c index 1bd1240d7..53e6b286e 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -76,7 +76,9 @@ __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)) */ +/* 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; @@ -89,6 +91,13 @@ typedef struct { 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) { @@ -247,8 +256,8 @@ static void frankenphp_restore_ini(void) { /* 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 = 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)) { @@ -285,7 +294,7 @@ static void frankenphp_snapshot_session_handlers(void) { } /* Check if user session handlers are defined */ - if (Z_ISUNDEF(PS(mod_user_names).ps_open)) { + if (Z_ISUNDEF(PS_MOD_USER_NAMES(ps_open))) { return; /* No user handlers to snapshot */ } @@ -295,12 +304,12 @@ static void frankenphp_snapshot_session_handlers(void) { } /* Copy each handler zval with incremented reference count */ -#define SNAPSHOT_HANDLER(name) \ - if (!Z_ISUNDEF(PS(mod_user_names).name)) { \ - ZVAL_COPY(&worker_session_handlers_snapshot->name, \ - &PS(mod_user_names).name); \ +#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->name); \ + ZVAL_UNDEF(&worker_session_handlers_snapshot->handler); \ } SNAPSHOT_HANDLER(ps_open); @@ -323,13 +332,13 @@ static void frankenphp_restore_session_handlers(void) { } /* Restore each handler zval */ -#define RESTORE_HANDLER(name) \ - if (!Z_ISUNDEF(worker_session_handlers_snapshot->name)) { \ - if (!Z_ISUNDEF(PS(mod_user_names).name)) { \ - zval_ptr_dtor(&PS(mod_user_names).name); \ +#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).name, \ - &worker_session_handlers_snapshot->name); \ + ZVAL_COPY(&PS_MOD_USER_NAMES(handler), \ + &worker_session_handlers_snapshot->handler); \ } RESTORE_HANDLER(ps_open); @@ -356,9 +365,9 @@ static void frankenphp_cleanup_worker_state(void) { /* Free session handlers snapshot */ if (worker_session_handlers_snapshot != NULL) { -#define FREE_HANDLER(name) \ - if (!Z_ISUNDEF(worker_session_handlers_snapshot->name)) { \ - zval_ptr_dtor(&worker_session_handlers_snapshot->name); \ +#define FREE_HANDLER(handler) \ + if (!Z_ISUNDEF(worker_session_handlers_snapshot->handler)) { \ + zval_ptr_dtor(&worker_session_handlers_snapshot->handler); \ } FREE_HANDLER(ps_open); diff --git a/frankenphp_test.go b/frankenphp_test.go index d3e96e1b5..83a151e44 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -341,7 +341,7 @@ func TestRequestSuperGlobalConditional_worker(t *testing.T) { runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) { if i%2 == 0 { // Even requests: don't use $_REQUEST - body, _:= testGet(fmt.Sprintf("http://example.com/request-superglobal-conditional.php?val=%d", i), handler, t) + body, _ := testGet(fmt.Sprintf("http://example.com/request-superglobal-conditional.php?val=%d", i), handler, t) assert.Contains(t, body, "SKIPPED") assert.Contains(t, body, fmt.Sprintf("'val' => '%d'", i)) } else { @@ -1135,7 +1135,7 @@ func TestSessionHandlerReset_worker(t *testing.T) { 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() + _ = resp1.Body.Close() body1Str := string(body1) assert.Contains(t, body1Str, "HANDLER_SET_AND_STARTED") @@ -1147,7 +1147,7 @@ func TestSessionHandlerReset_worker(t *testing.T) { 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() + _ = resp2.Body.Close() body2Str := string(body2) @@ -1179,7 +1179,7 @@ func TestIniLeakBetweenRequests_worker(t *testing.T) { resp1, err := http.Get(ts.URL + "/ini-leak.php?action=change_ini") assert.NoError(t, err) body1, _ := io.ReadAll(resp1.Body) - resp1.Body.Close() + _ = resp1.Body.Close() assert.Contains(t, string(body1), "INI_CHANGED") @@ -1187,7 +1187,7 @@ func TestIniLeakBetweenRequests_worker(t *testing.T) { resp2, err := http.Get(ts.URL + "/ini-leak.php?action=check_ini") assert.NoError(t, err) body2, _ := io.ReadAll(resp2.Body) - resp2.Body.Close() + _ = resp2.Body.Close() body2Str := string(body2) t.Logf("Response: %s", body2Str) @@ -1212,7 +1212,7 @@ func TestSessionHandlerPreLoopPreserved_worker(t *testing.T) { 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() + _ = resp1.Body.Close() body1Str := string(body1) t.Logf("Request 1 response: %s", body1Str) @@ -1225,7 +1225,7 @@ func TestSessionHandlerPreLoopPreserved_worker(t *testing.T) { 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() + _ = resp2.Body.Close() body2Str := string(body2) t.Logf("Request 2 response: %s", body2Str) @@ -1240,7 +1240,7 @@ func TestSessionHandlerPreLoopPreserved_worker(t *testing.T) { 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() + _ = resp3.Body.Close() body3Str := string(body3) t.Logf("Request 3 response: %s", body3Str) @@ -1261,7 +1261,7 @@ func TestIniPreLoopPreserved_worker(t *testing.T) { resp1, err := http.Get(ts.URL + "/worker-with-ini.php?action=check") assert.NoError(t, err) body1, _ := io.ReadAll(resp1.Body) - resp1.Body.Close() + _ = resp1.Body.Close() body1Str := string(body1) t.Logf("Request 1 response: %s", body1Str) @@ -1276,7 +1276,7 @@ func TestIniPreLoopPreserved_worker(t *testing.T) { 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() + _ = resp2.Body.Close() body2Str := string(body2) t.Logf("Request 2 response: %s", body2Str) @@ -1288,7 +1288,7 @@ func TestIniPreLoopPreserved_worker(t *testing.T) { resp3, err := http.Get(ts.URL + "/worker-with-ini.php?action=check") assert.NoError(t, err) body3, _ := io.ReadAll(resp3.Body) - resp3.Body.Close() + _ = resp3.Body.Close() body3Str := string(body3) t.Logf("Request 3 response: %s", body3Str) From fc1de8f67a8c0cda0914d2a47ee3a963950428cb Mon Sep 17 00:00:00 2001 From: Xavier Leune Date: Mon, 2 Feb 2026 17:19:55 +0100 Subject: [PATCH 3/4] fix(worker): comply with php-src coding style for memory allocation --- frankenphp.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/frankenphp.c b/frankenphp.c index 53e6b286e..6f3674b6c 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -298,10 +298,7 @@ static void frankenphp_snapshot_session_handlers(void) { 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 */ - } + worker_session_handlers_snapshot = emalloc(sizeof(session_user_handlers)); /* Copy each handler zval with incremented reference count */ #define SNAPSHOT_HANDLER(handler) \ @@ -382,7 +379,7 @@ static void frankenphp_cleanup_worker_state(void) { #undef FREE_HANDLER - free(worker_session_handlers_snapshot); + efree(worker_session_handlers_snapshot); worker_session_handlers_snapshot = NULL; } } From ca09e38312f1cfa396f073aea1e5717e5e93b499 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Mon, 2 Feb 2026 19:55:52 +0100 Subject: [PATCH 4/4] simplify --- frankenphp.c | 99 ++++++++++++++++++++++------------------------------ 1 file changed, 42 insertions(+), 57 deletions(-) diff --git a/frankenphp.c b/frankenphp.c index 6f3674b6c..cb3943772 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -98,6 +98,17 @@ typedef struct { #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) { @@ -201,7 +212,7 @@ static void frankenphp_release_temporary_streams() { } /* Destructor for INI snapshot hash table entries */ -static void ini_snapshot_dtor(zval *zv) { +static void frankenphp_ini_snapshot_dtor(zval *zv) { zend_string_release((zend_string *)Z_PTR_P(zv)); } @@ -212,13 +223,17 @@ static void frankenphp_snapshot_ini(void) { 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 */ + /* 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) { @@ -244,9 +259,10 @@ static void frankenphp_restore_ini(void) { /* 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; + 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; - size_t restore_capacity = 0; ZEND_HASH_FOREACH_STR_KEY_PTR(EG(modified_ini_directives), entry_name, ini_entry) { @@ -254,11 +270,6 @@ static void frankenphp_restore_ini(void) { 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. @@ -301,23 +312,14 @@ static void frankenphp_snapshot_session_handlers(void) { worker_session_handlers_snapshot = emalloc(sizeof(session_user_handlers)); /* 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)); \ +#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->handler); \ + ZVAL_UNDEF(&worker_session_handlers_snapshot->h); \ } - 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); + FOR_EACH_SESSION_HANDLER(SNAPSHOT_HANDLER); #undef SNAPSHOT_HANDLER } @@ -328,25 +330,16 @@ static void frankenphp_restore_session_handlers(void) { 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 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); \ } - 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); + FOR_EACH_SESSION_HANDLER(RESTORE_HANDLER); #undef RESTORE_HANDLER } @@ -362,20 +355,12 @@ static void frankenphp_cleanup_worker_state(void) { /* 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); +#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