-
-
Notifications
You must be signed in to change notification settings - Fork 194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
$_SERVER
lifecycle is unsafe in worker mode
#767
Comments
Thanks for the detailed report. Preserving the values of the last request will be necessary to allow features like the |
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) WDYT @withinboredom? |
I think that makes the most sense too. It's logically more consistent that way as well. |
#796) * test: failing test reproducing #767 * fix * Update frankenphp.c Co-authored-by: Tim Düsterhus <timwolla@googlemail.com> * Update frankenphp.c Co-authored-by: Tim Düsterhus <timwolla@googlemail.com> * review * ZVAL_COPY * fix * add back current $_SERVER behavior * add docs * bad fix for the leak * clean test * improve tests * fix test * fix * cleanup * clarify destroy super globals name * micro-optim: use zval_ptr_dtor_nogc to destroy super globals * style * fix * better name for frankenphp_free_server_context * more cleanup * remove useless memset * more cleanup * continue refactoring * fix and update docs * docs --------- Co-authored-by: Tim Düsterhus <timwolla@googlemail.com>
What happened?
Consider the following
worker.php
:Running the
worker.php
withingdb
as:and then making a request:
will output:
Note the following:
To userland code
$_SERVER
variable will preserve its request-specific contents afterfrankenphp_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.To internal code, the
$_SERVER
variable 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
beforefrankenphp_handle_request
should be identical to$_SERVER
afterfrankenphp_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
The text was updated successfully, but these errors were encountered: