Skip to content

Commit 61077e1

Browse files
authored
fix: clear in_save_handler state that blocks the subsequent close handler (#2443)
fixes #2368
1 parent e2a4abb commit 61077e1

3 files changed

Lines changed: 197 additions & 10 deletions

File tree

caddy/caddy_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package caddy_test
33
import (
44
"bytes"
55
"fmt"
6+
"io"
67
"net/http"
78
"os"
89
"path/filepath"
@@ -2010,3 +2011,50 @@ func TestSymlinkWorkerBehavior(t *testing.T) {
20102011
}
20112012
})
20122013
}
2014+
2015+
// TestSessionLockReleaseOnAbortInUserSaveHandler reproduces issue #2368: a
2016+
// timeout bailout inside a user save handler leaks the underlying flock.
2017+
// num_threads is 2 so R3 can't start until R1's thread frees, which
2018+
// guarantees R2 (blocked in flock) inherits the lock when R1 releases and
2019+
// then bails inside StrictSessionHandler. Without the fix R3 hangs in flock.
2020+
func TestSessionLockReleaseOnAbortInUserSaveHandler(t *testing.T) {
2021+
tester := caddytest.NewTester(t)
2022+
initServer(t, tester, `
2023+
{
2024+
skip_install_trust
2025+
admin localhost:2999
2026+
http_port `+testPort+`
2027+
https_port 9443
2028+
frankenphp {
2029+
num_threads 2
2030+
max_threads 2
2031+
}
2032+
}
2033+
localhost:`+testPort+` {
2034+
route {
2035+
php {
2036+
root ../testdata
2037+
}
2038+
}
2039+
}
2040+
`, "caddyfile")
2041+
2042+
client := &http.Client{Timeout: 5 * time.Second}
2043+
get := func() error {
2044+
resp, err := client.Get("http://localhost:" + testPort + "/session_deadlock.php")
2045+
if err != nil {
2046+
return err
2047+
}
2048+
_, _ = io.Copy(io.Discard, resp.Body)
2049+
return resp.Body.Close()
2050+
}
2051+
2052+
var wg sync.WaitGroup
2053+
wg.Add(2)
2054+
go func() { defer wg.Done(); _ = get() }() // R1: holder
2055+
time.Sleep(300 * time.Millisecond)
2056+
go func() { defer wg.Done(); _ = get() }() // R2: bails inside user save handler
2057+
time.Sleep(100 * time.Millisecond)
2058+
require.NoError(t, get(), "third request hung -- session lock leaked (issue #2368)")
2059+
wg.Wait()
2060+
}

frankenphp.c

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
#ifndef ZEND_WIN32
3030
#include <unistd.h>
3131
#endif
32-
#if defined(__linux__)
32+
#ifdef __linux__
3333
#include <sys/prctl.h>
3434
#elif defined(__FreeBSD__) || defined(__OpenBSD__)
3535
#include <pthread_np.h>
@@ -610,7 +610,7 @@ PHP_FUNCTION(frankenphp_request_headers) {
610610

611611
for (size_t i = 0; i < headers.r1; i++) {
612612
go_string key = headers.r0[i * 2];
613-
go_string val = headers.r0[i * 2 + 1];
613+
go_string val = headers.r0[(i * 2) + 1];
614614

615615
add_assoc_stringl_ex(return_value, key.data, key.len, val.data, val.len);
616616
}
@@ -1107,14 +1107,14 @@ frankenphp_register_variables_from_request_info(zval *track_vars_array) {
11071107
frankenphp_strings.content_type, (char *)SG(request_info).content_type,
11081108
true, track_vars_array);
11091109
frankenphp_register_variable_from_request_info(
1110-
frankenphp_strings.path_translated,
1111-
(char *)SG(request_info).path_translated, false, track_vars_array);
1110+
frankenphp_strings.path_translated, SG(request_info).path_translated,
1111+
false, track_vars_array);
11121112
frankenphp_register_variable_from_request_info(
11131113
frankenphp_strings.query_string, SG(request_info).query_string, true,
11141114
track_vars_array);
1115-
frankenphp_register_variable_from_request_info(
1116-
frankenphp_strings.remote_user, (char *)SG(request_info).auth_user, false,
1117-
track_vars_array);
1115+
frankenphp_register_variable_from_request_info(frankenphp_strings.remote_user,
1116+
SG(request_info).auth_user,
1117+
false, track_vars_array);
11181118
frankenphp_register_variable_from_request_info(
11191119
frankenphp_strings.request_method,
11201120
(char *)SG(request_info).request_method, false, track_vars_array);
@@ -1223,7 +1223,7 @@ sapi_module_struct frankenphp_sapi_module = {
12231223
* License: MIT
12241224
*/
12251225
static void set_thread_name(char *thread_name) {
1226-
#if defined(__linux__)
1226+
#ifdef __linux__
12271227
/* Use prctl instead to prevent using _GNU_SOURCE flag and implicit
12281228
* declaration */
12291229
prctl(PR_SET_NAME, thread_name);
@@ -1310,6 +1310,19 @@ static void *php_thread(void *arg) {
13101310

13111311
has_attempted_shutdown = true;
13121312

1313+
#ifdef HAVE_PHP_SESSION
1314+
/* A bailout inside a user save handler skips the cleanup that clears
1315+
* PS(in_save_handler); RSHUTDOWN's recursion guard then refuses to run
1316+
* the handler's close() and any resource it holds leaks
1317+
* (https://github.com/php/frankenphp/issues/2368). See
1318+
* https://github.com/php/php-src/blob/900797e54fb8d21a761205e5788b9275dc1c7c0e/ext/session/mod_user.c#L29
1319+
* fpm doesn't run into this because it kills timed out processes, which
1320+
* releases all resources:
1321+
* https://github.com/php/php-src/blob/900797e54fb8d21a761205e5788b9275dc1c7c0e/sapi/fpm/fpm/fpm_request.c#L276
1322+
*/
1323+
PS(in_save_handler) = false;
1324+
#endif
1325+
13131326
/* shutdown the request, potential bailout to zend_catch */
13141327
php_request_shutdown((void *)0);
13151328
frankenphp_free_request_context();
@@ -1678,8 +1691,8 @@ void frankenphp_destroy_thread_metrics(void) {
16781691
thread_metrics = NULL;
16791692
}
16801693

1681-
size_t frankenphp_get_thread_memory_usage(uintptr_t idx) {
1682-
return __atomic_load_n(&thread_metrics[idx].last_memory_usage,
1694+
size_t frankenphp_get_thread_memory_usage(uintptr_t thread_index) {
1695+
return __atomic_load_n(&thread_metrics[thread_index].last_memory_usage,
16831696
__ATOMIC_RELAXED);
16841697
}
16851698

testdata/session_deadlock.php

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
<?php
2+
3+
require_once __DIR__.'/_executor.php';
4+
5+
// Self-contained repro of https://github.com/php/frankenphp/issues/2368
6+
if (!class_exists('StrictSessionHandler', false)) {
7+
abstract class AbstractSessionHandler implements SessionHandlerInterface, SessionUpdateTimestampHandlerInterface
8+
{
9+
private string $sessionName;
10+
private string $prefetchId;
11+
private string $prefetchData;
12+
private ?string $newSessionId = null;
13+
14+
public function open(string $savePath, string $sessionName): bool
15+
{
16+
$this->sessionName = $sessionName;
17+
return true;
18+
}
19+
20+
abstract protected function doRead(string $sessionId): string;
21+
abstract protected function doWrite(string $sessionId, string $data): bool;
22+
abstract protected function doDestroy(string $sessionId): bool;
23+
24+
public function validateId(string $sessionId): bool
25+
{
26+
$this->prefetchData = $this->read($sessionId);
27+
$this->prefetchId = $sessionId;
28+
return '' !== $this->prefetchData;
29+
}
30+
31+
public function read(string $sessionId): string
32+
{
33+
if (isset($this->prefetchId)) {
34+
$prefetchId = $this->prefetchId;
35+
$prefetchData = $this->prefetchData;
36+
unset($this->prefetchId, $this->prefetchData);
37+
if ($prefetchId === $sessionId || '' === $prefetchData) {
38+
$this->newSessionId = '' === $prefetchData ? $sessionId : null;
39+
return $prefetchData;
40+
}
41+
}
42+
$data = $this->doRead($sessionId);
43+
$this->newSessionId = '' === $data ? $sessionId : null;
44+
return $data;
45+
}
46+
47+
public function write(string $sessionId, string $data): bool
48+
{
49+
$this->newSessionId = null;
50+
return $this->doWrite($sessionId, $data);
51+
}
52+
53+
public function destroy(string $sessionId): bool
54+
{
55+
return $this->newSessionId === $sessionId || $this->doDestroy($sessionId);
56+
}
57+
}
58+
59+
class StrictSessionHandler extends AbstractSessionHandler
60+
{
61+
public function __construct(private SessionHandlerInterface $handler) {}
62+
63+
public function open(string $savePath, string $sessionName): bool
64+
{
65+
parent::open($savePath, $sessionName);
66+
return $this->handler->open($savePath, $sessionName);
67+
}
68+
69+
public function updateTimestamp(string $sessionId, string $data): bool
70+
{
71+
return $this->write($sessionId, $data);
72+
}
73+
74+
protected function doRead(string $sessionId): string
75+
{
76+
return $this->handler->read($sessionId);
77+
}
78+
79+
protected function doWrite(string $sessionId, string $data): bool
80+
{
81+
return $this->handler->write($sessionId, $data);
82+
}
83+
84+
protected function doDestroy(string $sessionId): bool
85+
{
86+
return $this->handler->destroy($sessionId);
87+
}
88+
89+
public function close(): bool
90+
{
91+
return $this->handler->close();
92+
}
93+
94+
public function gc(int $maxlifetime): int|false
95+
{
96+
return $this->handler->gc($maxlifetime);
97+
}
98+
}
99+
}
100+
101+
return function () {
102+
// Pre-flock timer: any request that doesn't get the lock immediately will
103+
// have this fire while blocked in flock(), so when the lock is finally
104+
// released the request bails out *inside* StrictSessionHandler::doRead —
105+
// the path that leaks the fd without the fix from 2c6d2b5.
106+
set_time_limit(1);
107+
108+
$savePath = sys_get_temp_dir() . '/frankenphp-session-deadlock';
109+
@mkdir($savePath);
110+
ini_set('session.use_strict_mode', '1');
111+
ini_set('session.save_path', $savePath);
112+
session_set_save_handler(new StrictSessionHandler(new SessionHandler()), true);
113+
session_id('testsession2368');
114+
session_start();
115+
116+
// Reset the timer so the holder (and any waiter that successfully acquires
117+
// the lock) gets enough headroom to finish.
118+
set_time_limit(5);
119+
120+
echo "Done.\n";
121+
flush();
122+
// Hold the lock long enough that a 1s timer expires for any concurrent
123+
// waiter still blocked in flock().
124+
system('sleep 2');
125+
echo "ok!\n";
126+
};

0 commit comments

Comments
 (0)