fix(worker): revert ini reset, keep session fixes (#2139)

Revert the INI snapshot/restore mechanism from #2139 which caused
issues with frameworks that lazily set ini values like session.save_path
(#2185). Replace the session handler snapshot/restore with a simpler
direct session state reset from #2193, which preserves mod_user_names
across requests without requiring session module reload.

Co-Authored-By: Xavier Leune <xavier.leune@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

frankenphp.c

@@ -4,9 +4,11 @@
 #include <Zend/zend_interfaces.h>
 #include <Zend/zend_types.h>
 #include <errno.h>
-#include <ext/session/php_session.h>
 #include <ext/spl/spl_exceptions.h>
 #include <ext/standard/head.h>
+#ifdef HAVE_PHP_SESSION
+#include <ext/session/php_session.h>
+#endif
 #include <inttypes.h>
 #include <php.h>
 #include <php_config.h>
@@ -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;
-  }
-
-  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 */
-  }
-  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 = 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 */
-  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);                       \
-  }
-
-    FOR_EACH_SESSION_HANDLER(FREE_HANDLER);
-
-#undef FREE_HANDLER
-
-    efree(worker_session_handlers_snapshot);
-    worker_session_handlers_snapshot = NULL;
-  }
+#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);
+  }
+
+  if (!Z_ISUNDEF(PS(http_session_vars))) {
+    zval_ptr_dtor(&PS(http_session_vars));
+    ZVAL_UNDEF(&PS(http_session_vars));
+  }
+
+  if (PS(mod_data) || PS(mod_user_implemented)) {
+    zend_try { PS(mod)->s_close(&PS(mod_data)); }
+    zend_end_try();
+  }
+
+  if (PS(id)) {
+    zend_string_release_ex(PS(id), 0);
+    PS(id) = NULL;
+  }
+
+  if (PS(session_vars)) {
+    zend_string_release_ex(PS(session_vars), 0);
+    PS(session_vars) = NULL;
+  }
+
+  /* PS(mod_user_class_name) and PS(mod_user_names) are preserved */
+
+#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
+
+  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();
 }

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

testdata/ini-leak.php

@@ -1,67 +0,0 @@
-<?php
-
-require_once __DIR__.'/_executor.php';
-
-return function () {
-    $action = $_GET['action'] ?? 'default';
-    $output = [];
-
-    switch ($action) {
-        case 'change_ini':
-            // Change several INI values at runtime
-            $before = ini_get('display_errors');
-            ini_set('display_errors', '0');
-            $after = ini_get('display_errors');
-            $output[] = "display_errors: before=$before, after=$after";
-
-            $before = ini_get('max_execution_time');
-            ini_set('max_execution_time', '999');
-            $after = ini_get('max_execution_time');
-            $output[] = "max_execution_time: before=$before, after=$after";
-
-            $before = ini_get('precision');
-            ini_set('precision', '10');
-            $after = ini_get('precision');
-            $output[] = "precision: before=$before, after=$after";
-
-            $output[] = "INI_CHANGED";
-            break;
-
-        case 'check_ini':
-            // Check if INI values from previous request leaked
-            $display_errors = ini_get('display_errors');
-            $max_execution_time = ini_get('max_execution_time');
-            $precision = ini_get('precision');
-
-            $output[] = "display_errors=$display_errors";
-            $output[] = "max_execution_time=$max_execution_time";
-            $output[] = "precision=$precision";
-
-            // Check for leaks (values set in previous request)
-            $leaks = [];
-            if ($display_errors === '0') {
-                $leaks[] = "display_errors leaked (expected default, got 0)";
-            }
-            if ($max_execution_time === '999') {
-                $leaks[] = "max_execution_time leaked (expected default, got 999)";
-            }
-            if ($precision === '10') {
-                $leaks[] = "precision leaked (expected default, got 10)";
-            }
-
-            if (empty($leaks)) {
-                $output[] = "NO_LEAKS";
-            } else {
-                $output[] = "LEAKS_DETECTED";
-                foreach ($leaks as $leak) {
-                    $output[] = "LEAK: $leak";
-                }
-            }
-            break;
-
-        default:
-            $output[] = "UNKNOWN_ACTION";
-    }
-
-    echo implode("\n", $output);
-};

testdata/worker-with-ini.php

@@ -1,63 +0,0 @@
-<?php
-
-// Modify INI values BEFORE the worker loop (simulating framework setup)
-$preLoopPrecision = '8';
-$preLoopDisplayErrors = '0';
-
-ini_set('precision', $preLoopPrecision);
-ini_set('display_errors', $preLoopDisplayErrors);
-
-$requestCount = 0;
-
-do {
-    $ok = frankenphp_handle_request(function () use (
-        &$requestCount,
-        $preLoopPrecision,
-        $preLoopDisplayErrors
-    ): void {
-        $requestCount++;
-        $output = [];
-        $output[] = "request=$requestCount";
-
-        $action = $_GET['action'] ?? 'check';
-
-        switch ($action) {
-            case 'change_ini':
-                // Change INI values during the request
-                ini_set('precision', '5');
-                ini_set('display_errors', '1');
-                $output[] = "precision=" . ini_get('precision');
-                $output[] = "display_errors=" . ini_get('display_errors');
-                $output[] = "INI_CHANGED";
-                break;
-
-            case 'check':
-            default:
-                // Check if pre-loop INI values are preserved
-                $precision = ini_get('precision');
-                $displayErrors = ini_get('display_errors');
-
-                $output[] = "precision=$precision";
-                $output[] = "display_errors=$displayErrors";
-
-                $issues = [];
-                if ($precision !== $preLoopPrecision) {
-                    $issues[] = "precision mismatch (expected $preLoopPrecision)";
-                }
-                if ($displayErrors !== $preLoopDisplayErrors) {
-                    $issues[] = "display_errors mismatch (expected $preLoopDisplayErrors)";
-                }
-
-                if (empty($issues)) {
-                    $output[] = "PRELOOP_INI_PRESERVED";
-                } else {
-                    $output[] = "PRELOOP_INI_LOST";
-                    foreach ($issues as $issue) {
-                        $output[] = "ISSUE: $issue";
-                    }
-                }
-        }
-
-        echo implode("\n", $output);
-    });
-} while ($ok);