fix: shield worker error handlers from request-parsing warnings - #2632
fix: shield worker error handlers from request-parsing warnings#2632dunglas wants to merge 1 commit into
Conversation
In worker mode, request parsing (post_max_size, max_input_vars, multipart errors...) happens inside frankenphp_handle_request(), where the worker script's persistent userland error handler is installed. A throwing handler (symfony/error-handler) turned these warnings into an uncaught ErrorException, killing the worker script and answering the request with a bogus 200: any unauthenticated client could restart workers at will. The userland error handler is now bypassed while PHP parses the request, restoring classic SAPI semantics: the warning goes to the log and the callback runs with truncated input. The diagnostics raised during this window are recorded and exposed through the new frankenphp_request_parse_errors() function (entries shaped like error_get_last()), in both worker and classic modes, so applications can reject such requests with a 4xx status instead of processing truncated input. Fixes #2631
|
TBH not a fan of the error_get_last() request flow. Wouldn't it be possible to just do something like this? while($continue) {
try {
$continue = frankenphp_handle_request($handler)
} catch(\Exception $e) {
// handle error
}
} |
|
If we plan to modify the worker api, I'd rather do something that gives us more control, like this: $server = new \FrankenPHP\Server()
$server->onRequest(function() {
// ...
})->onMessage(function() {
// ...
})->onError(function() {
// ...
});
$server->run();
|
|
Your first suggestion would be a BC break. I like the second one but it's a brand new API, the benefit of the current approach is that it's BC break free and inline with existing PHP functions. If we introduce a new API, it could even be: It could also be nice to add a hook to execute code after a request. |
It would just be a change on the Symfony side. But I realized that the try-catch does nothing since the exception is already caught by frankenphp_log("starting up");
set_error_handler(function ($errno, $errstr, $errfile, $errline) {
throw new Exception($errstr);
});
while (frankenphp_handle_request(function(){
echo "Hello World";
})) {}To get the same behavior as with FPM, you'd need to not forward the error and instead reset the the error handler briefly on each request. Kind of what you were already doing, but passing the error is unnecessary. |
Fixes #2631
Problem
In worker mode, request parsing (
post_max_sizeoverflow,max_input_varsexceeded, malformed multipart payloads...) happens insidefrankenphp_handle_request(), between two requests, where the worker script's persistent userland error handler is live. Under a throwing handler (symfony/error-handler, i.e. every stock Symfony app in worker mode), the E_WARNING becomes an uncaughtErrorException: the callback is never invoked, the worker script dies and restarts, and the client receives a 200 with an empty or fatal-dump body. Any unauthenticated client can restart workers at will by POSTing a form with more thanmax_input_varsfields.Classic SAPIs never had this problem because input parsing runs before the script starts, when no userland error handler exists.
Fix
The userland error handler is bypassed while PHP parses the request. During the startup parse window,
EG(user_error_handler)is temporarily unset so every diagnostic routes tozend_error_cband reaches the log, exactly as under PHP-FPM or Apache. The callback then runs with truncated input, matching classic SAPI behavior. The restore mirrors zend's own pattern (Zend/zend.c): if userland code running inside the window (e.g.session.auto_startwith a user save handler) registers a new handler, the new handler wins.Diagnostics raised during that window are recorded by a
zend_error_cbwrapper (installed once perInit()on the PHP main thread; recording is gated by a thread-local flag, and the wrapper always chains to the original callback). Entries are stored withmalloc/strdupso they survive bailouts and stay invisible to the per-request allocator.New PHP function
frankenphp_request_parse_errors(): arrayexposes them, shaped likeerror_get_last()({type, message, file, line}), empty array on a cleanly parsed request. Works in both worker and classic modes, so frameworks can reject unparsable requests with a 400/413 instead of processing truncated input:Notes
frankenphp_arginfo.hregenerated with php-src'sgen_stub.php(PHP-8.5 branch), which also modernized the existingZEND_FALIASentries toZEND_RAW_FENTRY.FrankenPhpWorkerRunner.Tests:
TestWorkerSurvivesRequestParseErrorsreproduces the exact Symfony scenario (throwing ErrorException handler) and proves worker survival across both failure types via a request counter, plus reset between requests;TestRequestParseErrors_modulecovers classic mode.