frankenphp
frankenphp copied to clipboard
php
`$_SERVER` lifecycle is unsafe in worker mode
What happened?
Consider the following worker.php:
<?php
ignore_user_abort(true);
$_SERVER['MAX_REQUESTS'] ??= 1000;
error_log(print_r($_SERVER, true));
for($nbRequests = 0, $running = true; isset($_SERVER['MAX_REQUESTS']) && ($nbRequests < ((int)$_SERVER['MAX_REQUESTS'])) && $running; ++$nbRequests) {
error_log(print_r($_SERVER, true));
$running = \frankenphp_handle_request(static function () {
echo "Hello World!";
});
error_log(print_r($_SERVER, true));
getopt('abc');
}
Running the worker.php within gdb as:
gdb --args ../caddy/frankenphp/frankenphp php-server --worker worker.php
and then making a request:
curl -v http://localhost/worker.php?some_query_parameters
will output:
2024/05/06 14:04:52.779 INFO Array
(
[HOSTNAME] => c3b166044495
[SHLVL] => 1
[HOME] => /root
[OLDPWD] => /go/src/app
[GOTOOLCHAIN] => local
[LOGNAME] => root
[_] => /usr/bin/gdb
[TERM] => xterm
[COLUMNS] => 339
[PATH] => /go/bin:/usr/local/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
[CFLAGS] => -ggdb3
[GOPATH] => /go
[PHPIZE_DEPS] => autoconf dpkg-dev file g++ gcc libc-dev make pkg-config re2c
[PWD] => /go/src/app/testdata
[LINES] => 96
[GOLANG_VERSION] => 1.22.2
[AUTH_TYPE] =>
[REMOTE_IDENT] =>
[QUERY_STRING] =>
[REQUEST_METHOD] => GET
[REQUEST_URI] => worker.php
[CONTENT_LENGTH] =>
[DOCUMENT_ROOT] => /go/src/app/testdata
[DOCUMENT_URI] => worker.php
[GATEWAY_INTERFACE] => CGI/1.1
[HTTP_HOST] =>
[HTTPS] =>
[PATH_INFO] =>
[PHP_SELF] => worker.php
[REMOTE_ADDR] =>
[REMOTE_HOST] =>
[REMOTE_PORT] =>
[REQUEST_SCHEME] => http
[SCRIPT_FILENAME] => /go/src/app/testdata/worker.php
[SCRIPT_NAME] => /worker.php
[SERVER_NAME] =>
[SERVER_PORT] => 80
[SERVER_PROTOCOL] => HTTP/1.1
[SERVER_SOFTWARE] => FrankenPHP
[SSL_PROTOCOL] =>
[FRANKENPHP_WORKER] => 1 {"syslog_level": "notice"}
2024/05/06 14:04:52.779 INFO FrankenPHP started 🐘 {"php_version": "8.3.8-dev"}
2024/05/06 14:04:52.780 INFO Caddy serving PHP app on :80
2024/05/06 14:04:52.790 WARN tls storage cleaning happened too recently; skipping for now {"storage": "FileStorage:/root/.local/share/caddy", "instance": "61794d2e-90a3-449d-8ea5-842c5e837ac7", "try_again": "2024/05/07 14:04:52.790", "try_again_in": 86399.999999544}
2024/05/06 14:04:52.790 INFO tls finished cleaning storage units
2024/05/06 14:04:58.884 INFO Array
(
[HOSTNAME] => c3b166044495
[SHLVL] => 1
[HOME] => /root
[OLDPWD] => /go/src/app
[GOTOOLCHAIN] => local
[LOGNAME] => root
[_] => /usr/bin/gdb
[TERM] => xterm
[COLUMNS] => 339
[PATH] => /go/bin:/usr/local/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
[CFLAGS] => -ggdb3
[GOPATH] => /go
[PHPIZE_DEPS] => autoconf dpkg-dev file g++ gcc libc-dev make pkg-config re2c
[PWD] => /go/src/app/testdata
[LINES] => 96
[GOLANG_VERSION] => 1.22.2
[AUTH_TYPE] =>
[REMOTE_IDENT] =>
[QUERY_STRING] => some_query_parameters
[REQUEST_METHOD] => GET
[REQUEST_URI] => /worker.php?some_query_parameters
[CONTENT_LENGTH] =>
[DOCUMENT_ROOT] => /go/src/app/testdata
[DOCUMENT_URI] => worker.php
[GATEWAY_INTERFACE] => CGI/1.1
[HTTP_HOST] => localhost
[HTTPS] =>
[PATH_INFO] =>
[PHP_SELF] => worker.php
[REMOTE_ADDR] => 127.0.0.1
[REMOTE_HOST] => 127.0.0.1
[REMOTE_PORT] => 38618
[REQUEST_SCHEME] => http
[SCRIPT_FILENAME] => /go/src/app/testdata/worker.php
[SCRIPT_NAME] => /worker.php
[SERVER_NAME] => localhost
[SERVER_PORT] => 80
[SERVER_PROTOCOL] => HTTP/1.1
[SERVER_SOFTWARE] => FrankenPHP
[SSL_PROTOCOL] =>
[HTTP_USER_AGENT] => curl/7.88.1
[HTTP_ACCEPT] => */*
[REQUEST_TIME_FLOAT] => 1715004298.8845
[REQUEST_TIME] => 1715004298
)
{"syslog_level": "notice"}
Thread 16 "thpool-0" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff81ffb6c0 (LWP 802)]
0x00007ffff73485f4 in _zend_is_inconsistent (ht=0x0, file=0x7ffff7d71548 "/usr/local/src/php/Zend/zend_hash.c", line=2681) at /usr/local/src/php/Zend/zend_hash.c:57
57 if ((HT_FLAGS(ht) & HASH_FLAG_CONSISTENCY) == HT_OK) {
(gdb) bt
#0 0x00007ffff73485f4 in _zend_is_inconsistent (ht=0x0, file=0x7ffff7d71548 "/usr/local/src/php/Zend/zend_hash.c", line=2681) at /usr/local/src/php/Zend/zend_hash.c:57
#1 0x00007ffff7351600 in zend_hash_find_known_hash (ht=0x0, key=0x7fff60202980) at /usr/local/src/php/Zend/zend_hash.c:2681
#2 0x00007ffff717eb62 in zend_hash_find_ex (ht=0x0, key=0x7fff60202980, known_hash=true) at /usr/local/src/php/Zend/zend_hash.h:188
#3 0x00007ffff717ec78 in zend_hash_find_ex_ind (ht=0x0, key=0x7fff60202980, known_hash=true) at /usr/local/src/php/Zend/zend_hash.h:427
#4 0x00007ffff71857f8 in zif_getopt (execute_data=0x7fff804150c0, return_value=0x7fff81ff9410) at /usr/local/src/php/ext/standard/basic_functions.c:979
#5 0x00007ffff7377773 in ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER () at /usr/local/src/php/Zend/zend_vm_execute.h:1275
#6 0x00007ffff73faa64 in execute_ex (ex=0x7fff80415020) at /usr/local/src/php/Zend/zend_vm_execute.h:57212
#7 0x00007ffff73ff357 in zend_execute (op_array=0x7fff80477000, return_value=0x0) at /usr/local/src/php/Zend/zend_vm_execute.h:61604
#8 0x00007ffff7332f07 in zend_execute_scripts (type=8, retval=0x0, file_count=3) at /usr/local/src/php/Zend/zend.c:1891
#9 0x00007ffff726bb0c in php_execute_script (primary_file=0x7fff81ffaa90) at /usr/local/src/php/main/main.c:2515
#10 0x00000000017eb2d9 in frankenphp_execute_script (file_name=0x0) at frankenphp.c:851
#11 0x00000000017e8977 in _cgo_b7d6fd74a0c8_Cfunc_frankenphp_execute_script (v=0xc000805dd8) at /tmp/go-build/cgo-gcc-prolog:55
#12 0x000000000047e764 in runtime.asmcgocall () at /usr/local/go/src/runtime/asm_amd64.s:918
#13 0x000000c000800700 in ?? ()
#14 0x00007fff81ffac00 in ?? ()
#15 0x0000000000000000 in ?? ()
Note the following:
-
To userland code
$_SERVERvariable will preserve its request-specific contents afterfrankenphp_handle_requestreturns. This might or might not be intended. To the developer it might be a gotcha, because they might expect the request callback to be sandboxed and not affect the logic outside of it. I did not see anything about this in the documentation. -
To internal code, the
$_SERVERvariable will be unavailable after the first call tofrankenphp_handle_request, because thePG(http_globals)are cleared infrankenphp_request_reset(). This makes it unsafe to call any internal function that relies on superglobals, as demonstrated by the call togetopt():
https://github.com/php/php-src/blob/55966f098b23fe34a526d64f984c191bdc53b1e7/ext/standard/basic_functions.c#L953-L956
For the fix, I don't have a strong preference whether $_SERVER before frankenphp_handle_request should be identical to $_SERVER after frankenphp_handle_request, or whether the request-specific data should be preserved in $_SERVER as it is the case. I believe it should be documented, though. Furthermore the view for internal functions should be consistent with the userland view, i.e. calling internal functions relying on $_SERVER should not lead to a crash and observe the same contents for the array.
Build Type
dev.Dockerfile
Worker Mode
Yes
Operating System
GNU/Linux
CPU Architecture
x86_64
PHP configuration
dev.Dockerfile without any config changes.
Relevant log output
No response
Thanks for the detailed report.
Preserving the values of the last request will be necessary to allow features like the Kernel::terminate() provided by Symfony and Laravel.
Preserving the values of the last request will be necessary to allow features like the
Kernel::terminate()provided by Symfony and Laravel.
Makes sense. Then this should be documented and the support for native functions fixed.
Let me know if you need any additional information that I didn't provide in the initial issue report.
After a second thought, I think that it would be better to restore the original (non-request-bound) $_SERVER when leaving the closure.
Symfony and Laravel call fastcgi_finish_request() (that we also support in FrankenPHP) as soon as possible, so we don't have to worry about the "terminate" feature, we can call it in the closure: https://github.com/search?q=repo%3Asymfony%2Fsymfony+fastcgi_finish_request&type=code
WDYT @withinboredom?
I think that makes the most sense too. It's logically more consistent that way as well.
