Skip to content

Commit fb3536f

Browse files
committed
Fix leak+crash with sapi_windows_set_ctrl_handler()
The ctrl_handler is never destroyed. We have to destroy it at request end so we avoid leaking it and also avoid keeping a reference to previous request memory in a next request. The latter can result in a crash and can be demonstrated with this script and `--repeat 2`: ```php class Test { public function set() { sapi_windows_set_ctrl_handler(self::cb(...)); } public function cb() { } } $test = new Test; $test->set(); sleep(3); ``` When you hit CTRL+C in the second request you can crash. This patch resolves both the leak and crash by destroying the ctrl_handler after a request. Closes GH-18231.
1 parent 8a58585 commit fb3536f

File tree

5 files changed

+55
-2
lines changed

5 files changed

+55
-2
lines changed

NEWS

+3
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,9 @@ PHP NEWS
3232
- Standard:
3333
. Fixed bug GH-17403 (Potential deadlock when putenv fails). (nielsdos)
3434

35+
- Windows:
36+
. Fix leak+crash with sapi_windows_set_ctrl_handler(). (nielsdos)
37+
3538
- Zip:
3639
. Fixed bug GH-18431 (Registering ZIP progress callback twice doesn't work).
3740
(nielsdos)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
--TEST--
2+
sapi_windows_set_ctrl_handler() leak bug
3+
--SKIPIF--
4+
<?php
5+
include "skipif.inc";
6+
7+
if (strtoupper(substr(PHP_OS, 0, 3)) !== 'WIN')
8+
die("skip this test is for Windows platforms only");
9+
?>
10+
--FILE--
11+
<?php
12+
13+
class Test {
14+
public function set() {
15+
sapi_windows_set_ctrl_handler(self::cb(...));
16+
}
17+
public function cb() {
18+
}
19+
}
20+
21+
$test = new Test;
22+
$test->set();
23+
24+
echo "Done\n";
25+
26+
?>
27+
--EXPECT--
28+
Done

win32/globals.c

+2
Original file line numberDiff line numberDiff line change
@@ -63,5 +63,7 @@ PHP_RSHUTDOWN_FUNCTION(win32_core_globals)
6363
{/*{{{*/
6464
closelog();
6565

66+
php_win32_signal_ctrl_handler_request_shutdown();
67+
6668
return SUCCESS;
6769
}/*}}}*/

win32/signal.c

+21-2
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,28 @@ PHP_WINUTIL_API void php_win32_signal_ctrl_handler_shutdown(void)
6868
zend_interrupt_function = orig_interrupt_function;
6969
orig_interrupt_function = NULL;
7070
vm_interrupt_flag = NULL;
71-
ZVAL_UNDEF(&ctrl_handler);
7271
}/*}}}*/
7372

73+
PHP_WINUTIL_API void php_win32_signal_ctrl_handler_request_shutdown(void)
74+
{
75+
/* Must be initialized and in main thread */
76+
if (!vm_interrupt_flag) {
77+
return;
78+
}
79+
#ifdef ZTS
80+
if (!tsrm_is_main_thread()) {
81+
return;
82+
}
83+
#endif
84+
85+
/* The ctrl_handler must be cleared between requests, otherwise we can crash
86+
* due to accessing a previous request's memory. */
87+
if (!Z_ISUNDEF(ctrl_handler)) {
88+
zval_ptr_dtor(&ctrl_handler);
89+
ZVAL_UNDEF(&ctrl_handler);
90+
}
91+
}
92+
7493
static BOOL WINAPI php_win32_signal_system_ctrl_handler(DWORD evt)
7594
{/*{{{*/
7695
if (CTRL_C_EVENT != evt && CTRL_BREAK_EVENT != evt) {
@@ -125,7 +144,7 @@ PHP_FUNCTION(sapi_windows_set_ctrl_handler)
125144
RETURN_FALSE;
126145
}
127146

128-
zval_ptr_dtor_nogc(&ctrl_handler);
147+
zval_ptr_dtor(&ctrl_handler);
129148
ZVAL_COPY(&ctrl_handler, &fci.function_name);
130149

131150
RETURN_TRUE;

win32/signal.h

+1
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#define SIGPROF 27 /* profiling time alarm */
1111

1212
PHP_WINUTIL_API void php_win32_signal_ctrl_handler_init(void);
13+
PHP_WINUTIL_API void php_win32_signal_ctrl_handler_request_shutdown(void);
1314
PHP_WINUTIL_API void php_win32_signal_ctrl_handler_shutdown(void);
1415

1516
#endif /* PHP_WIN32_SIGNAL_H */

0 commit comments

Comments
 (0)