search

dunglas / frankenphp

🧟 The modern PHP app server
https://frankenphp.dev
MIT License
6.7k stars 220 forks source link

`$_SERVER` lifecycle is unsafe in worker mode #767

Closed TimWolla closed 3 months ago

TimWolla commented 4 months ago

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:

  1. To userland code $_SERVER variable will preserve its request-specific contents after frankenphp_handle_request returns. 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.

  2. To internal code, the $_SERVER variable will be unavailable after the first call to frankenphp_handle_request, because the PG(http_globals) are cleared in frankenphp_request_reset(). This makes it unsafe to call any internal function that relies on superglobals, as demonstrated by the call to getopt():

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

dunglas commented 4 months ago

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.

TimWolla commented 4 months ago

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.

dunglas commented 4 months ago

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?

withinboredom commented 4 months ago

I think that makes the most sense too. It's logically more consistent that way as well.