Skip to content

Commit 0ee1c3b

Browse files
AlliBalliBabadunglas
AlliBalliBaba
authored and
dunglas
committed
Fixes session handlers.
1 parent f068912 commit 0ee1c3b

3 files changed

Lines changed: 40 additions & 203 deletions

File tree

‎frankenphp.c‎

Lines changed: 0 additions & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
#include <Zend/zend_interfaces.h>
55
#include <Zend/zend_types.h>
66
#include <errno.h>
7-
#include <ext/session/php_session.h>
87
#include <ext/spl/spl_exceptions.h>
98
#include <ext/standard/head.h>
109
#include <inttypes.h>
@@ -76,41 +75,6 @@ __thread bool is_worker_thread = false;
7675
__thread zval *os_environment = NULL;
7776
__thread HashTable *worker_ini_snapshot = NULL;
7877

79-
/* Session user handler names (same structure as PS(mod_user_names)).
80-
* In PHP 8.2, mod_user_names is a union with .name.ps_* access.
81-
* In PHP 8.3+, mod_user_names is a direct struct with .ps_* access. */
82-
typedef struct {
83-
zval ps_open;
84-
zval ps_close;
85-
zval ps_read;
86-
zval ps_write;
87-
zval ps_destroy;
88-
zval ps_gc;
89-
zval ps_create_sid;
90-
zval ps_validate_sid;
91-
zval ps_update_timestamp;
92-
} session_user_handlers;
93-
94-
/* Macro to access PS(mod_user_names) handlers across PHP versions */
95-
#if PHP_VERSION_ID >= 80300
96-
#define PS_MOD_USER_NAMES(handler) PS(mod_user_names).handler
97-
#else
98-
#define PS_MOD_USER_NAMES(handler) PS(mod_user_names).name.handler
99-
#endif
100-
101-
#define FOR_EACH_SESSION_HANDLER(op) \
102-
op(ps_open); \
103-
op(ps_close); \
104-
op(ps_read); \
105-
op(ps_write); \
106-
op(ps_destroy); \
107-
op(ps_gc); \
108-
op(ps_create_sid); \
109-
op(ps_validate_sid); \
110-
op(ps_update_timestamp)
111-
112-
__thread session_user_handlers *worker_session_handlers_snapshot = NULL;
113-
11478
void frankenphp_update_local_thread_context(bool is_worker) {
11579
is_worker_thread = is_worker;
11680
}
@@ -155,13 +119,6 @@ static void frankenphp_reset_super_globals() {
155119
zval *files = &PG(http_globals)[TRACK_VARS_FILES];
156120
zval_ptr_dtor_nogc(files);
157121
memset(files, 0, sizeof(*files));
158-
159-
/* $_SESSION must be explicitly deleted from the symbol table.
160-
* Unlike other superglobals, $_SESSION is stored in EG(symbol_table)
161-
* with a reference to PS(http_session_vars). The session RSHUTDOWN
162-
* only decrements the refcount but doesn't remove it from the symbol
163-
* table, causing data to leak between requests. */
164-
zend_hash_str_del(&EG(symbol_table), "_SESSION", sizeof("_SESSION") - 1);
165122
}
166123
zend_end_try();
167124

@@ -300,59 +257,6 @@ static void frankenphp_restore_ini(void) {
300257
}
301258
}
302259

303-
/* Save session user handlers set before the worker loop.
304-
* This allows frameworks to define custom session handlers that persist. */
305-
static void frankenphp_snapshot_session_handlers(void) {
306-
if (worker_session_handlers_snapshot != NULL) {
307-
return; /* Already snapshotted */
308-
}
309-
310-
/* Check if session module is loaded */
311-
if (zend_hash_str_find_ptr(&module_registry, "session",
312-
sizeof("session") - 1) == NULL) {
313-
return; /* Session module not available */
314-
}
315-
316-
/* Check if user session handlers are defined */
317-
if (Z_ISUNDEF(PS_MOD_USER_NAMES(ps_open))) {
318-
return; /* No user handlers to snapshot */
319-
}
320-
321-
worker_session_handlers_snapshot = emalloc(sizeof(session_user_handlers));
322-
323-
/* Copy each handler zval with incremented reference count */
324-
#define SNAPSHOT_HANDLER(h) \
325-
if (!Z_ISUNDEF(PS_MOD_USER_NAMES(h))) { \
326-
ZVAL_COPY(&worker_session_handlers_snapshot->h, &PS_MOD_USER_NAMES(h)); \
327-
} else { \
328-
ZVAL_UNDEF(&worker_session_handlers_snapshot->h); \
329-
}
330-
331-
FOR_EACH_SESSION_HANDLER(SNAPSHOT_HANDLER);
332-
333-
#undef SNAPSHOT_HANDLER
334-
}
335-
336-
/* Restore session user handlers from snapshot after RSHUTDOWN freed them. */
337-
static void frankenphp_restore_session_handlers(void) {
338-
if (worker_session_handlers_snapshot == NULL) {
339-
return;
340-
}
341-
342-
/* Restore each handler zval.
343-
* Session RSHUTDOWN already freed the handlers via zval_ptr_dtor and set
344-
* them to UNDEF, so we don't need to destroy them again. We simply copy
345-
* from the snapshot (which holds its own reference). */
346-
#define RESTORE_HANDLER(h) \
347-
if (!Z_ISUNDEF(worker_session_handlers_snapshot->h)) { \
348-
ZVAL_COPY(&PS_MOD_USER_NAMES(h), &worker_session_handlers_snapshot->h); \
349-
}
350-
351-
FOR_EACH_SESSION_HANDLER(RESTORE_HANDLER);
352-
353-
#undef RESTORE_HANDLER
354-
}
355-
356260
/* Free worker state when the worker script terminates. */
357261
static void frankenphp_cleanup_worker_state(void) {
358262
/* Free INI snapshot */
@@ -361,21 +265,6 @@ static void frankenphp_cleanup_worker_state(void) {
361265
FREE_HASHTABLE(worker_ini_snapshot);
362266
worker_ini_snapshot = NULL;
363267
}
364-
365-
/* Free session handlers snapshot */
366-
if (worker_session_handlers_snapshot != NULL) {
367-
#define FREE_HANDLER(h) \
368-
if (!Z_ISUNDEF(worker_session_handlers_snapshot->h)) { \
369-
zval_ptr_dtor(&worker_session_handlers_snapshot->h); \
370-
}
371-
372-
FOR_EACH_SESSION_HANDLER(FREE_HANDLER);
373-
374-
#undef FREE_HANDLER
375-
376-
efree(worker_session_handlers_snapshot);
377-
worker_session_handlers_snapshot = NULL;
378-
}
379268
}
380269

381270
/* Adapted from php_request_shutdown */
@@ -416,7 +305,6 @@ bool frankenphp_shutdown_dummy_request(void) {
416305
* The framework has set these up before the worker loop, and we want
417306
* to preserve them. Session RSHUTDOWN will free the handlers. */
418307
frankenphp_snapshot_ini();
419-
frankenphp_snapshot_session_handlers();
420308

421309
frankenphp_worker_request_shutdown();
422310

@@ -488,12 +376,6 @@ static int frankenphp_worker_request_startup() {
488376
module->request_startup_func(module->type, module->module_number);
489377
}
490378
}
491-
492-
/* Restore session handlers AFTER session RINIT.
493-
* Session RSHUTDOWN frees mod_user_names callbacks, so we must restore
494-
* them before user code runs. This must happen after RINIT because
495-
* session RINIT may reset some state. */
496-
frankenphp_restore_session_handlers();
497379
}
498380
zend_catch { retval = FAILURE; }
499381
zend_end_try();

‎frankenphp_test.go‎

Lines changed: 25 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1207,47 +1207,31 @@ func TestIniLeakBetweenRequests_worker(t *testing.T) {
12071207
})
12081208
}
12091209

1210-
func TestSessionHandlerPreLoopPreserved_worker(t *testing.T) {
1211-
runTest(t, func(_ func(http.ResponseWriter, *http.Request), ts *httptest.Server, i int) {
1212-
// Request 1: Check that the pre-loop session handler is preserved
1213-
resp1, err := http.Get(ts.URL + "/worker-with-session-handler.php?action=check")
1214-
assert.NoError(t, err)
1215-
body1, _ := io.ReadAll(resp1.Body)
1216-
_ = resp1.Body.Close()
1217-
1218-
body1Str := string(body1)
1219-
t.Logf("Request 1 response: %s", body1Str)
1220-
assert.Contains(t, body1Str, "HANDLER_PRESERVED",
1221-
"Session handler set before loop should be preserved")
1222-
assert.Contains(t, body1Str, "save_handler=user",
1223-
"session.save_handler should remain 'user'")
1224-
1225-
// Request 2: Use the session - should work with pre-loop handler
1226-
resp2, err := http.Get(ts.URL + "/worker-with-session-handler.php?action=use_session")
1227-
assert.NoError(t, err)
1228-
body2, _ := io.ReadAll(resp2.Body)
1229-
_ = resp2.Body.Close()
1230-
1231-
body2Str := string(body2)
1232-
t.Logf("Request 2 response: %s", body2Str)
1233-
assert.Contains(t, body2Str, "SESSION_OK",
1234-
"Session should work with pre-loop handler.\nResponse: %s", body2Str)
1235-
assert.NotContains(t, body2Str, "ERROR:",
1236-
"No errors expected.\nResponse: %s", body2Str)
1237-
assert.NotContains(t, body2Str, "EXCEPTION:",
1238-
"No exceptions expected.\nResponse: %s", body2Str)
1239-
1240-
// Request 3: Check handler is still preserved after using session
1241-
resp3, err := http.Get(ts.URL + "/worker-with-session-handler.php?action=check")
1242-
assert.NoError(t, err)
1243-
body3, _ := io.ReadAll(resp3.Body)
1244-
_ = resp3.Body.Close()
1245-
1246-
body3Str := string(body3)
1247-
t.Logf("Request 3 response: %s", body3Str)
1248-
assert.Contains(t, body3Str, "HANDLER_PRESERVED",
1249-
"Session handler should still be preserved after use")
1250-
1210+
func TestSessionHandler_worker(t *testing.T) {
1211+
runTest(t, func(handler func(http.ResponseWriter, *http.Request), ts *httptest.Server, i int) {
1212+
url := ts.URL + "/worker-with-session-handler.php"
1213+
body, _ := testGet(url+"?action=check", handler, t)
1214+
assert.Contains(t, body, "no session value", "request without session cookie, should not see session value")
1215+
1216+
body, responseWithSessionCookie := testGet(url+"?action=read_session", handler, t)
1217+
assert.Contains(t, body, "no session value")
1218+
assert.Len(t, responseWithSessionCookie.Header["Set-Cookie"], 1, "Expected exactly one Set-Cookie header")
1219+
1220+
requestWithSessionCookie := httptest.NewRequest("GET", url+"?action=put_session", nil)
1221+
requestWithSessionCookie.Header["Cookie"] = []string{responseWithSessionCookie.Header["Set-Cookie"][0]}
1222+
body, _ = testRequest(requestWithSessionCookie, handler, t)
1223+
assert.Contains(t, body, "session value set", "make request with session cookie, should see session value set in previous request")
1224+
1225+
body, _ = testGet( url+"?action=read_session", handler, t)
1226+
assert.Contains(t, body, "no session value", "make request without session cookie, should not see previous session value")
1227+
1228+
body, _ = testGet( url+"?action=check", handler, t)
1229+
assert.Contains(t, body, "no session value", "make request without starting a session")
1230+
1231+
requestWithSessionCookie = httptest.NewRequest("GET", url+"?action=read_session", nil)
1232+
requestWithSessionCookie.Header["Cookie"] = []string{responseWithSessionCookie.Header["Set-Cookie"][0]}
1233+
body, _ = testRequest( requestWithSessionCookie, handler, t)
1234+
assert.Contains(t, body, "session value exists", "make final request with session cookie, should see previous value")
12511235
}, &testOptions{
12521236
workerScript: "worker-with-session-handler.php",
12531237
nbWorkers: 1,

‎testdata/worker-with-session-handler.php‎

Lines changed: 15 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ public function read(string $id): string|false
2222

2323
public function write(string $id, string $data): bool
2424
{
25+
echo "WRITING SESSION: id=$id, data=$data\n";
2526
self::$data[$id] = $data;
2627
return true;
2728
}
@@ -40,59 +41,29 @@ public function gc(int $max_lifetime): int|false
4041

4142
// Set the session handler BEFORE the worker loop
4243
$handler = new PreLoopSessionHandler();
43-
session_set_save_handler($handler, true);
44-
45-
$requestCount = 0;
4644

4745
do {
48-
$ok = frankenphp_handle_request(function () use (&$requestCount): void {
49-
$requestCount++;
50-
$output = [];
51-
$output[] = "request=$requestCount";
52-
46+
$ok = frankenphp_handle_request(function () use ($handler): void {
5347
$action = $_GET['action'] ?? 'check';
5448

5549
switch ($action) {
56-
case 'use_session':
57-
// Try to use the session - should work with pre-loop handler
58-
$error = null;
59-
set_error_handler(function ($errno, $errstr) use (&$error) {
60-
$error = $errstr;
61-
return true;
62-
});
63-
64-
try {
65-
session_id('test-preloop-' . $requestCount);
66-
$result = session_start();
67-
if ($result) {
68-
$_SESSION['test'] = 'value-' . $requestCount;
69-
session_write_close();
70-
$output[] = "SESSION_OK";
71-
} else {
72-
$output[] = "SESSION_START_FAILED";
73-
}
74-
} catch (Throwable $e) {
75-
$output[] = "EXCEPTION:" . $e->getMessage();
76-
}
77-
78-
restore_error_handler();
79-
80-
if ($error) {
81-
$output[] = "ERROR:" . $error;
82-
}
50+
case 'put_session':
51+
session_set_save_handler($handler, true);
52+
session_start();
53+
$_SESSION['test'] = 'session value exists';
54+
echo 'session value set';
55+
break;
56+
case 'read_session':
57+
session_set_save_handler($handler, true);
58+
session_start();
59+
echo 'session id:' . session_id();
60+
echo $_SESSION['test'] ?? 'no session value';
8361
break;
8462

8563
case 'check':
8664
default:
87-
$saveHandler = ini_get('session.save_handler');
88-
$output[] = "save_handler=$saveHandler";
89-
if ($saveHandler === 'user') {
90-
$output[] = "HANDLER_PRESERVED";
91-
} else {
92-
$output[] = "HANDLER_LOST";
93-
}
65+
echo $_SESSION['test'] ?? 'no session value';
66+
break;
9467
}
95-
96-
echo implode("\n", $output);
9768
});
9869
} while ($ok);

0 commit comments

Comments
 (0)