Skip to content

Fix #78413: php-fpm request_terminate_timeout does not take effect af… #4637

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix #78413: php-fpm request_terminate_timeout does not take effect af… #4637

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57e6699
Fix #78413: php-fpm request_terminate_timeout does not take effect af…
turchanov Aug 28, 2019
088d9f0
Changes suggested by code reviewer (nikic). We have settled for a more
turchanov Sep 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions sapi/fpm/fpm/fpm_conf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ static struct ini_value_parser_s ini_fpm_pool_options[] = {
{ "request_slowlog_timeout", &fpm_conf_set_time, WPO(request_slowlog_timeout) },
{ "request_slowlog_trace_depth", &fpm_conf_set_integer, WPO(request_slowlog_trace_depth) },
{ "request_terminate_timeout", &fpm_conf_set_time, WPO(request_terminate_timeout) },
{ "request_terminate_strict", &fpm_conf_set_boolean, WPO(request_terminate_strict) },
{ "rlimit_files", &fpm_conf_set_integer, WPO(rlimit_files) },
{ "rlimit_core", &fpm_conf_set_rlimit_core, WPO(rlimit_core) },
{ "chroot", &fpm_conf_set_string, WPO(chroot) },
Expand Down Expand Up @@ -1664,6 +1665,7 @@ static void fpm_conf_dump() /* {{{ */
zlog(ZLOG_NOTICE, "\trequest_slowlog_timeout = %ds", wp->config->request_slowlog_timeout);
zlog(ZLOG_NOTICE, "\trequest_slowlog_trace_depth = %d", wp->config->request_slowlog_trace_depth);
zlog(ZLOG_NOTICE, "\trequest_terminate_timeout = %ds", wp->config->request_terminate_timeout);
zlog(ZLOG_NOTICE, "\trequest_terminate_strict = %s", BOOL2STR(wp->config->request_terminate_strict));
zlog(ZLOG_NOTICE, "\trlimit_files = %d", wp->config->rlimit_files);
zlog(ZLOG_NOTICE, "\trlimit_core = %d", wp->config->rlimit_core);
zlog(ZLOG_NOTICE, "\tchroot = %s", STR2STR(wp->config->chroot));
Expand Down
1 change: 1 addition & 0 deletions sapi/fpm/fpm/fpm_conf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ struct fpm_worker_pool_config_s {
int request_slowlog_timeout;
int request_slowlog_trace_depth;
int request_terminate_timeout;
int request_terminate_strict;
int rlimit_files;
int rlimit_core;
char *chroot;
Expand Down
3 changes: 2 additions & 1 deletion sapi/fpm/fpm/fpm_process_ctl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,13 +293,14 @@ static void fpm_pctl_check_request_timeout(struct timeval *now) /* {{{ */
struct fpm_worker_pool_s *wp;

for (wp = fpm_worker_all_pools; wp; wp = wp->next) {
int strict = wp->config->request_terminate_strict;
int terminate_timeout = wp->config->request_terminate_timeout;
int slowlog_timeout = wp->config->request_slowlog_timeout;
struct fpm_child_s *child;

if (terminate_timeout || slowlog_timeout) {
for (child = wp->children; child; child = child->next) {
fpm_request_check_timed_out(child, now, terminate_timeout, slowlog_timeout);
fpm_request_check_timed_out(child, now, terminate_timeout, slowlog_timeout, strict);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions sapi/fpm/fpm/fpm_request.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ void fpm_request_finished() /* {{{ */
}
/* }}} */

void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *now, int terminate_timeout, int slowlog_timeout) /* {{{ */
void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *now, int terminate_timeout, int slowlog_timeout, int strict) /* {{{ */
{
struct fpm_scoreboard_proc_s proc, *proc_p;

Expand All @@ -244,7 +244,7 @@ void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *now,
}
#endif

if (proc.request_stage > FPM_REQUEST_ACCEPTING && proc.request_stage < FPM_REQUEST_END) {
if (proc.request_stage > FPM_REQUEST_ACCEPTING && ((proc.request_stage < FPM_REQUEST_END) || strict)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make the timeout also apply in the FINISHED state. Is that right?

Additionally this also controls the slow log behavior...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make the timeout also apply in the FINISHED state. Is that right?

Yes, exactly. That was the intention. proc.request_stage will be set to FINISHED either:

  • (usually) in a request processing loop after php_execute_script finished executing current script, a call to fpm_request_end is made which in turn sets FPM_REQUEST_FINISHED
  • (prematurely) by fastcgi_finish_request if FastCGI keep-alive is not active (as far as I know none of major webservers implement FastCGI keep-alive) via callchain: fastcgi_finish_request -> fcgi_close -> if (... !req->keep ... ) -> req->hook.on_close = fpm_request_finished -> FPM_REQUEST_FINISHED.

And because requests in FPM_REQUEST_FINISHED are no longer tracked for timeout, this creates an opportunity for PHP code to seize control of worker process for arbitrary amount of time.
In addition to fastcgi_finish_request, there is another way to exploit this bug by using shutdown functions (via register_shutdown_function) since shutdown handlers are called in php_request_shutdown which is called when request stage is already FINISHED.

After a careful review I came to conclusion that I might need to add a check for proc.request_stage <= FPM_REQUEST_FINISHED just in case if some newer states will be added in future... What do you think? But from the other hand it will create a possibility to evade timeout timits in future ...

Additionally this also controls the slow log behavior...

Slow log explicitly checks for proc.request_stage == FPM_REQUEST_EXECUTING so that even if we broaden set of request stages in parent if it will be ok.

php-src/sapi/fpm/fpm/fpm_request.c

Lines 254 to 257 in 1ac6b02

if (child->slow_logged.tv_sec == 0 && slowlog_timeout &&
proc.request_stage == FPM_REQUEST_EXECUTING && tv.tv_sec >= slowlog_timeout) {
str_purify_filename(purified_script_filename, proc.script_filename, sizeof(proc.script_filename));

char purified_script_filename[sizeof(proc.script_filename)];
struct timeval tv;

Expand Down
2 changes: 1 addition & 1 deletion sapi/fpm/fpm/fpm_request.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ void fpm_request_finished(); /* request processed: cleaning current request *
struct fpm_child_s;
struct timeval;

void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *tv, int terminate_timeout, int slowlog_timeout);
void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *tv, int terminate_timeout, int slowlog_timeout, int strict);
int fpm_request_is_idle(struct fpm_child_s *child);
const char *fpm_request_get_stage_name(int stage);
int fpm_request_last_activity(struct fpm_child_s *child, struct timeval *tv);
Expand Down
6 changes: 6 additions & 0 deletions sapi/fpm/www.conf.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,12 @@ pm.max_spare_servers = 3
; Default Value: 0
;request_terminate_timeout = 0

; When application calls fastcgi_finish_request it will inhibit request
; termination after request_terminate_timeout time limit. This option will enable
; timeout limit to be applied unconditionally even after fastcgi_finish_request.
; To retain legacy behavior the default is 'no'
;request_terminate_strict = no

; Set open file descriptor rlimit.
; Default Value: system defined value
;rlimit_files = 1024
Expand Down