From 29fe06fa5919bb0f239677d29f3856d64537eeec Mon Sep 17 00:00:00 2001 From: Till Backhaus Date: Mon, 19 Mar 2018 13:06:04 +0100 Subject: [PATCH] Fix bug #76109: Implement fpm_scoreboard_copy fpm_scoreboard_copy locks the scoreboard while copying the scoreboard and all proc scoreboards. proc scoreboards are locked one by one while copying each struct. The old implementation (inside fpm_handle_status_request) only briefly locked the scoreboard while copying the scorebard. Closes GH-7931 Co-authored-by: Jakub Zelenka --- NEWS | 4 ++ sapi/fpm/fpm/fpm_scoreboard.c | 57 +++++++++++++++++ sapi/fpm/fpm/fpm_scoreboard.h | 6 ++ sapi/fpm/fpm/fpm_status.c | 112 ++++++++++++++++------------------ 4 files changed, 118 insertions(+), 61 deletions(-) diff --git a/NEWS b/NEWS index 64bb448e262ef..3e1d12d0fc9b0 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,10 @@ PHP NEWS - GD: . Fixed libpng warning when loading interlaced images. (Brett) +- FPM: + . Fixed bug #76109 (Unsafe access to fpm scoreboard). + (Till Backhaus, Jakub Zelenka) + - Iconv: . Fixed bug GH-7953 (ob_clean() only does not set Content-Encoding). (cmb) . Fixed bug GH-7980 (Unexpected result for iconv_mime_decode). (cmb) diff --git a/sapi/fpm/fpm/fpm_scoreboard.c b/sapi/fpm/fpm/fpm_scoreboard.c index 5380d184901bf..ec28af49d7c4b 100644 --- a/sapi/fpm/fpm/fpm_scoreboard.c +++ b/sapi/fpm/fpm/fpm_scoreboard.c @@ -226,6 +226,63 @@ void fpm_scoreboard_release(struct fpm_scoreboard_s *scoreboard) { scoreboard->lock = 0; } +struct fpm_scoreboard_s *fpm_scoreboard_copy(struct fpm_scoreboard_s *scoreboard, int copy_procs) +{ + struct fpm_scoreboard_s *scoreboard_copy; + struct fpm_scoreboard_proc_s *scoreboard_proc_p; + size_t scoreboard_size, scoreboard_nprocs_size; + int i; + void *mem; + + if (!scoreboard) { + scoreboard = fpm_scoreboard_get(); + } + + if (copy_procs) { + scoreboard_size = sizeof(struct fpm_scoreboard_s); + scoreboard_nprocs_size = sizeof(struct fpm_scoreboard_proc_s) * scoreboard->nprocs; + + mem = malloc(scoreboard_size + scoreboard_nprocs_size); + } else { + mem = malloc(sizeof(struct fpm_scoreboard_s)); + } + + if (!mem) { + zlog(ZLOG_ERROR, "scoreboard: failed to allocate memory for copy"); + return NULL; + } + + scoreboard_copy = mem; + + scoreboard = fpm_scoreboard_acquire(scoreboard, FPM_SCOREBOARD_LOCK_NOHANG); + if (!scoreboard) { + free(mem); + zlog(ZLOG_ERROR, "scoreboard: failed to lock (already locked)"); + return NULL; + } + + *scoreboard_copy = *scoreboard; + + if (copy_procs) { + mem += scoreboard_size; + + for (i = 0; i < scoreboard->nprocs; i++, mem += sizeof(struct fpm_scoreboard_proc_s)) { + scoreboard_proc_p = fpm_scoreboard_proc_acquire(scoreboard, i, FPM_SCOREBOARD_LOCK_HANG); + scoreboard_copy->procs[i] = *scoreboard_proc_p; + fpm_scoreboard_proc_release(scoreboard_proc_p); + } + } + + fpm_scoreboard_release(scoreboard); + + return scoreboard_copy; +} + +void fpm_scoreboard_free_copy(struct fpm_scoreboard_s *scoreboard) +{ + free(scoreboard); +} + struct fpm_scoreboard_proc_s *fpm_scoreboard_proc_acquire(struct fpm_scoreboard_s *scoreboard, int child_index, int nohang) /* {{{ */ { struct fpm_scoreboard_proc_s *proc; diff --git a/sapi/fpm/fpm/fpm_scoreboard.h b/sapi/fpm/fpm/fpm_scoreboard.h index 363bdd260547c..e22d6d933a00a 100644 --- a/sapi/fpm/fpm/fpm_scoreboard.h +++ b/sapi/fpm/fpm/fpm_scoreboard.h @@ -15,6 +15,9 @@ #define FPM_SCOREBOARD_ACTION_SET 0 #define FPM_SCOREBOARD_ACTION_INC 1 +#define FPM_SCOREBOARD_LOCK_HANG 0 +#define FPM_SCOREBOARD_LOCK_NOHANG 1 + struct fpm_scoreboard_proc_s { union { atomic_t lock; @@ -87,6 +90,9 @@ void fpm_scoreboard_child_use(struct fpm_child_s *child, pid_t pid); void fpm_scoreboard_proc_free(struct fpm_child_s *child); int fpm_scoreboard_proc_alloc(struct fpm_child_s *child); +struct fpm_scoreboard_s *fpm_scoreboard_copy(struct fpm_scoreboard_s *scoreboard, int copy_procs); +void fpm_scoreboard_free_copy(struct fpm_scoreboard_s *scoreboard); + #ifdef HAVE_TIMES float fpm_scoreboard_get_tick(); #endif diff --git a/sapi/fpm/fpm/fpm_status.c b/sapi/fpm/fpm/fpm_status.c index 0d74f551b7970..b77c7fb700aa0 100644 --- a/sapi/fpm/fpm/fpm_status.c +++ b/sapi/fpm/fpm/fpm_status.c @@ -136,8 +136,8 @@ int fpm_status_export_to_zval(zval *status) int fpm_status_handle_request(void) /* {{{ */ { - struct fpm_scoreboard_s scoreboard, *scoreboard_p; - struct fpm_scoreboard_proc_s proc; + struct fpm_scoreboard_s *scoreboard_p; + struct fpm_scoreboard_proc_s *proc; char *buffer, *time_format, time_buffer[64]; time_t now_epoch; int full, encode; @@ -170,9 +170,16 @@ int fpm_status_handle_request(void) /* {{{ */ if (fpm_status_uri && !strcmp(fpm_status_uri, SG(request_info).request_uri)) { fpm_request_executing(); + /* full status ? */ + _GET_str = zend_string_init(ZEND_STRL("_GET"), 0); + full = (fpm_php_get_string_from_table(_GET_str, "full") != NULL); + short_syntax = short_post = NULL; + full_separator = full_pre = full_syntax = full_post = NULL; + encode = 0; + scoreboard_p = fpm_scoreboard_get(); - if (scoreboard_p->shared) { - scoreboard_p = scoreboard_p->shared; + if (scoreboard_p) { + scoreboard_p = fpm_scoreboard_copy(scoreboard_p->shared ? scoreboard_p->shared : scoreboard_p, full); } if (!scoreboard_p) { zlog(ZLOG_ERROR, "status: unable to find or access status shared memory"); @@ -184,21 +191,9 @@ int fpm_status_handle_request(void) /* {{{ */ return 1; } - if (!fpm_spinlock(&scoreboard_p->lock, 1)) { - zlog(ZLOG_NOTICE, "[pool %s] status: scoreboard already in used.", scoreboard_p->pool); - SG(sapi_headers).http_response_code = 503; - sapi_add_header_ex(ZEND_STRL("Content-Type: text/plain"), 1, 1); - sapi_add_header_ex(ZEND_STRL("Expires: Thu, 01 Jan 1970 00:00:00 GMT"), 1, 1); - sapi_add_header_ex(ZEND_STRL("Cache-Control: no-cache, no-store, must-revalidate, max-age=0"), 1, 1); - PUTS("Server busy. Please try again later."); - return 1; - } - /* copy the scoreboard not to bother other processes */ - scoreboard = *scoreboard_p; - fpm_unlock(scoreboard_p->lock); - - if (scoreboard.idle < 0 || scoreboard.active < 0) { - zlog(ZLOG_ERROR, "[pool %s] invalid status values", scoreboard.pool); + if (scoreboard_p->idle < 0 || scoreboard_p->active < 0) { + fpm_scoreboard_free_copy(scoreboard_p); + zlog(ZLOG_ERROR, "[pool %s] invalid status values", scoreboard_p->pool); SG(sapi_headers).http_response_code = 500; sapi_add_header_ex(ZEND_STRL("Content-Type: text/plain"), 1, 1); sapi_add_header_ex(ZEND_STRL("Expires: Thu, 01 Jan 1970 00:00:00 GMT"), 1, 1); @@ -214,16 +209,10 @@ int fpm_status_handle_request(void) /* {{{ */ /* handle HEAD */ if (SG(request_info).headers_only) { + fpm_scoreboard_free_copy(scoreboard_p); return 1; } - /* full status ? */ - _GET_str = zend_string_init("_GET", sizeof("_GET")-1, 0); - full = (fpm_php_get_string_from_table(_GET_str, "full") != NULL); - short_syntax = short_post = NULL; - full_separator = full_pre = full_syntax = full_post = NULL; - encode = 0; - /* HTML */ if (fpm_php_get_string_from_table(_GET_str, "html")) { sapi_add_header_ex(ZEND_STRL("Content-Type: text/html"), 1, 1); @@ -429,23 +418,23 @@ int fpm_status_handle_request(void) /* {{{ */ } } - strftime(time_buffer, sizeof(time_buffer) - 1, time_format, localtime(&scoreboard.start_epoch)); + strftime(time_buffer, sizeof(time_buffer) - 1, time_format, localtime(&scoreboard_p->start_epoch)); now_epoch = time(NULL); spprintf(&buffer, 0, short_syntax, - scoreboard.pool, - PM2STR(scoreboard.pm), + scoreboard_p->pool, + PM2STR(scoreboard_p->pm), time_buffer, - (unsigned long) (now_epoch - scoreboard.start_epoch), - scoreboard.requests, - scoreboard.lq, - scoreboard.lq_max, - scoreboard.lq_len, - scoreboard.idle, - scoreboard.active, - scoreboard.idle + scoreboard.active, - scoreboard.active_max, - scoreboard.max_children_reached, - scoreboard.slow_rq); + (unsigned long) (now_epoch - scoreboard_p->start_epoch), + scoreboard_p->requests, + scoreboard_p->lq, + scoreboard_p->lq_max, + scoreboard_p->lq_len, + scoreboard_p->idle, + scoreboard_p->active, + scoreboard_p->idle + scoreboard_p->active, + scoreboard_p->active_max, + scoreboard_p->max_children_reached, + scoreboard_p->slow_rq); PUTS(buffer); efree(buffer); @@ -475,7 +464,7 @@ int fpm_status_handle_request(void) /* {{{ */ if (!scoreboard_p->procs[i].used) { continue; } - proc = scoreboard_p->procs[i]; + proc = &scoreboard_p->procs[i]; if (first) { first = 0; @@ -487,44 +476,44 @@ int fpm_status_handle_request(void) /* {{{ */ query_string = NULL; tmp_query_string = NULL; - if (proc.query_string[0] != '\0') { + if (proc->query_string[0] != '\0') { if (!encode) { - query_string = proc.query_string; + query_string = proc->query_string; } else { - tmp_query_string = php_escape_html_entities_ex((const unsigned char *) proc.query_string, strlen(proc.query_string), 1, ENT_HTML_IGNORE_ERRORS & ENT_COMPAT, NULL, /* double_encode */ 1, /* quiet */ 0); + tmp_query_string = php_escape_html_entities_ex((const unsigned char *) proc->query_string, strlen(proc->query_string), 1, ENT_HTML_IGNORE_ERRORS & ENT_COMPAT, NULL, /* double_encode */ 1, /* quiet */ 0); query_string = ZSTR_VAL(tmp_query_string); } } /* prevent NaN */ - if (proc.cpu_duration.tv_sec == 0 && proc.cpu_duration.tv_usec == 0) { + if (proc->cpu_duration.tv_sec == 0 && proc->cpu_duration.tv_usec == 0) { cpu = 0.; } else { - cpu = (proc.last_request_cpu.tms_utime + proc.last_request_cpu.tms_stime + proc.last_request_cpu.tms_cutime + proc.last_request_cpu.tms_cstime) / fpm_scoreboard_get_tick() / (proc.cpu_duration.tv_sec + proc.cpu_duration.tv_usec / 1000000.) * 100.; + cpu = (proc->last_request_cpu.tms_utime + proc->last_request_cpu.tms_stime + proc->last_request_cpu.tms_cutime + proc->last_request_cpu.tms_cstime) / fpm_scoreboard_get_tick() / (proc->cpu_duration.tv_sec + proc->cpu_duration.tv_usec / 1000000.) * 100.; } - if (proc.request_stage == FPM_REQUEST_ACCEPTING) { - duration = proc.duration; + if (proc->request_stage == FPM_REQUEST_ACCEPTING) { + duration = proc->duration; } else { - timersub(&now, &proc.accepted, &duration); + timersub(&now, &proc->accepted, &duration); } - strftime(time_buffer, sizeof(time_buffer) - 1, time_format, localtime(&proc.start_epoch)); + strftime(time_buffer, sizeof(time_buffer) - 1, time_format, localtime(&proc->start_epoch)); spprintf(&buffer, 0, full_syntax, - (int) proc.pid, - fpm_request_get_stage_name(proc.request_stage), + (int) proc->pid, + fpm_request_get_stage_name(proc->request_stage), time_buffer, - (unsigned long) (now_epoch - proc.start_epoch), - proc.requests, + (unsigned long) (now_epoch - proc->start_epoch), + proc->requests, duration.tv_sec * 1000000UL + duration.tv_usec, - proc.request_method[0] != '\0' ? proc.request_method : "-", - proc.request_uri[0] != '\0' ? proc.request_uri : "-", + proc->request_method[0] != '\0' ? proc->request_method : "-", + proc->request_uri[0] != '\0' ? proc->request_uri : "-", query_string ? "?" : "", query_string ? query_string : "", - proc.content_length, - proc.auth_user[0] != '\0' ? proc.auth_user : "-", - proc.script_filename[0] != '\0' ? proc.script_filename : "-", - proc.request_stage == FPM_REQUEST_ACCEPTING ? cpu : 0., - proc.request_stage == FPM_REQUEST_ACCEPTING ? proc.memory : 0); + proc->content_length, + proc->auth_user[0] != '\0' ? proc->auth_user : "-", + proc->script_filename[0] != '\0' ? proc->script_filename : "-", + proc->request_stage == FPM_REQUEST_ACCEPTING ? cpu : 0., + proc->request_stage == FPM_REQUEST_ACCEPTING ? proc->memory : 0); PUTS(buffer); efree(buffer); @@ -538,6 +527,7 @@ int fpm_status_handle_request(void) /* {{{ */ } } + fpm_scoreboard_free_copy(scoreboard_p); return 1; }