Skip to content

Commit 29fe06f

Browse files
Till Backhausbukka
Till Backhaus
and
bukka
committed
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 <bukka@php.net>
1 parent c035298 commit 29fe06f

File tree

4 files changed

+118
-61
lines changed

4 files changed

+118
-61
lines changed

‎NEWS

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,10 @@ PHP NEWS
88
- GD:
99
. Fixed libpng warning when loading interlaced images. (Brett)
1010

11+
- FPM:
12+
. Fixed bug #76109 (Unsafe access to fpm scoreboard).
13+
(Till Backhaus, Jakub Zelenka)
14+
1115
- Iconv:
1216
. Fixed bug GH-7953 (ob_clean() only does not set Content-Encoding). (cmb)
1317
. Fixed bug GH-7980 (Unexpected result for iconv_mime_decode). (cmb)

‎sapi/fpm/fpm/fpm_scoreboard.c

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,63 @@ void fpm_scoreboard_release(struct fpm_scoreboard_s *scoreboard) {
226226
scoreboard->lock = 0;
227227
}
228228

229+
struct fpm_scoreboard_s *fpm_scoreboard_copy(struct fpm_scoreboard_s *scoreboard, int copy_procs)
230+
{
231+
struct fpm_scoreboard_s *scoreboard_copy;
232+
struct fpm_scoreboard_proc_s *scoreboard_proc_p;
233+
size_t scoreboard_size, scoreboard_nprocs_size;
234+
int i;
235+
void *mem;
236+
237+
if (!scoreboard) {
238+
scoreboard = fpm_scoreboard_get();
239+
}
240+
241+
if (copy_procs) {
242+
scoreboard_size = sizeof(struct fpm_scoreboard_s);
243+
scoreboard_nprocs_size = sizeof(struct fpm_scoreboard_proc_s) * scoreboard->nprocs;
244+
245+
mem = malloc(scoreboard_size + scoreboard_nprocs_size);
246+
} else {
247+
mem = malloc(sizeof(struct fpm_scoreboard_s));
248+
}
249+
250+
if (!mem) {
251+
zlog(ZLOG_ERROR, "scoreboard: failed to allocate memory for copy");
252+
return NULL;
253+
}
254+
255+
scoreboard_copy = mem;
256+
257+
scoreboard = fpm_scoreboard_acquire(scoreboard, FPM_SCOREBOARD_LOCK_NOHANG);
258+
if (!scoreboard) {
259+
free(mem);
260+
zlog(ZLOG_ERROR, "scoreboard: failed to lock (already locked)");
261+
return NULL;
262+
}
263+
264+
*scoreboard_copy = *scoreboard;
265+
266+
if (copy_procs) {
267+
mem += scoreboard_size;
268+
269+
for (i = 0; i < scoreboard->nprocs; i++, mem += sizeof(struct fpm_scoreboard_proc_s)) {
270+
scoreboard_proc_p = fpm_scoreboard_proc_acquire(scoreboard, i, FPM_SCOREBOARD_LOCK_HANG);
271+
scoreboard_copy->procs[i] = *scoreboard_proc_p;
272+
fpm_scoreboard_proc_release(scoreboard_proc_p);
273+
}
274+
}
275+
276+
fpm_scoreboard_release(scoreboard);
277+
278+
return scoreboard_copy;
279+
}
280+
281+
void fpm_scoreboard_free_copy(struct fpm_scoreboard_s *scoreboard)
282+
{
283+
free(scoreboard);
284+
}
285+
229286
struct fpm_scoreboard_proc_s *fpm_scoreboard_proc_acquire(struct fpm_scoreboard_s *scoreboard, int child_index, int nohang) /* {{{ */
230287
{
231288
struct fpm_scoreboard_proc_s *proc;

‎sapi/fpm/fpm/fpm_scoreboard.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@
1515
#define FPM_SCOREBOARD_ACTION_SET 0
1616
#define FPM_SCOREBOARD_ACTION_INC 1
1717

18+
#define FPM_SCOREBOARD_LOCK_HANG 0
19+
#define FPM_SCOREBOARD_LOCK_NOHANG 1
20+
1821
struct fpm_scoreboard_proc_s {
1922
union {
2023
atomic_t lock;
@@ -87,6 +90,9 @@ void fpm_scoreboard_child_use(struct fpm_child_s *child, pid_t pid);
8790
void fpm_scoreboard_proc_free(struct fpm_child_s *child);
8891
int fpm_scoreboard_proc_alloc(struct fpm_child_s *child);
8992

93+
struct fpm_scoreboard_s *fpm_scoreboard_copy(struct fpm_scoreboard_s *scoreboard, int copy_procs);
94+
void fpm_scoreboard_free_copy(struct fpm_scoreboard_s *scoreboard);
95+
9096
#ifdef HAVE_TIMES
9197
float fpm_scoreboard_get_tick();
9298
#endif

‎sapi/fpm/fpm/fpm_status.c

Lines changed: 51 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -136,8 +136,8 @@ int fpm_status_export_to_zval(zval *status)
136136

137137
int fpm_status_handle_request(void) /* {{{ */
138138
{
139-
struct fpm_scoreboard_s scoreboard, *scoreboard_p;
140-
struct fpm_scoreboard_proc_s proc;
139+
struct fpm_scoreboard_s *scoreboard_p;
140+
struct fpm_scoreboard_proc_s *proc;
141141
char *buffer, *time_format, time_buffer[64];
142142
time_t now_epoch;
143143
int full, encode;
@@ -170,9 +170,16 @@ int fpm_status_handle_request(void) /* {{{ */
170170
if (fpm_status_uri && !strcmp(fpm_status_uri, SG(request_info).request_uri)) {
171171
fpm_request_executing();
172172

173+
/* full status ? */
174+
_GET_str = zend_string_init(ZEND_STRL("_GET"), 0);
175+
full = (fpm_php_get_string_from_table(_GET_str, "full") != NULL);
176+
short_syntax = short_post = NULL;
177+
full_separator = full_pre = full_syntax = full_post = NULL;
178+
encode = 0;
179+
173180
scoreboard_p = fpm_scoreboard_get();
174-
if (scoreboard_p->shared) {
175-
scoreboard_p = scoreboard_p->shared;
181+
if (scoreboard_p) {
182+
scoreboard_p = fpm_scoreboard_copy(scoreboard_p->shared ? scoreboard_p->shared : scoreboard_p, full);
176183
}
177184
if (!scoreboard_p) {
178185
zlog(ZLOG_ERROR, "status: unable to find or access status shared memory");
@@ -184,21 +191,9 @@ int fpm_status_handle_request(void) /* {{{ */
184191
return 1;
185192
}
186193

187-
if (!fpm_spinlock(&scoreboard_p->lock, 1)) {
188-
zlog(ZLOG_NOTICE, "[pool %s] status: scoreboard already in used.", scoreboard_p->pool);
189-
SG(sapi_headers).http_response_code = 503;
190-
sapi_add_header_ex(ZEND_STRL("Content-Type: text/plain"), 1, 1);
191-
sapi_add_header_ex(ZEND_STRL("Expires: Thu, 01 Jan 1970 00:00:00 GMT"), 1, 1);
192-
sapi_add_header_ex(ZEND_STRL("Cache-Control: no-cache, no-store, must-revalidate, max-age=0"), 1, 1);
193-
PUTS("Server busy. Please try again later.");
194-
return 1;
195-
}
196-
/* copy the scoreboard not to bother other processes */
197-
scoreboard = *scoreboard_p;
198-
fpm_unlock(scoreboard_p->lock);
199-
200-
if (scoreboard.idle < 0 || scoreboard.active < 0) {
201-
zlog(ZLOG_ERROR, "[pool %s] invalid status values", scoreboard.pool);
194+
if (scoreboard_p->idle < 0 || scoreboard_p->active < 0) {
195+
fpm_scoreboard_free_copy(scoreboard_p);
196+
zlog(ZLOG_ERROR, "[pool %s] invalid status values", scoreboard_p->pool);
202197
SG(sapi_headers).http_response_code = 500;
203198
sapi_add_header_ex(ZEND_STRL("Content-Type: text/plain"), 1, 1);
204199
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) /* {{{ */
214209

215210
/* handle HEAD */
216211
if (SG(request_info).headers_only) {
212+
fpm_scoreboard_free_copy(scoreboard_p);
217213
return 1;
218214
}
219215

220-
/* full status ? */
221-
_GET_str = zend_string_init("_GET", sizeof("_GET")-1, 0);
222-
full = (fpm_php_get_string_from_table(_GET_str, "full") != NULL);
223-
short_syntax = short_post = NULL;
224-
full_separator = full_pre = full_syntax = full_post = NULL;
225-
encode = 0;
226-
227216
/* HTML */
228217
if (fpm_php_get_string_from_table(_GET_str, "html")) {
229218
sapi_add_header_ex(ZEND_STRL("Content-Type: text/html"), 1, 1);
@@ -429,23 +418,23 @@ int fpm_status_handle_request(void) /* {{{ */
429418
}
430419
}
431420

432-
strftime(time_buffer, sizeof(time_buffer) - 1, time_format, localtime(&scoreboard.start_epoch));
421+
strftime(time_buffer, sizeof(time_buffer) - 1, time_format, localtime(&scoreboard_p->start_epoch));
433422
now_epoch = time(NULL);
434423
spprintf(&buffer, 0, short_syntax,
435-
scoreboard.pool,
436-
PM2STR(scoreboard.pm),
424+
scoreboard_p->pool,
425+
PM2STR(scoreboard_p->pm),
437426
time_buffer,
438-
(unsigned long) (now_epoch - scoreboard.start_epoch),
439-
scoreboard.requests,
440-
scoreboard.lq,
441-
scoreboard.lq_max,
442-
scoreboard.lq_len,
443-
scoreboard.idle,
444-
scoreboard.active,
445-
scoreboard.idle + scoreboard.active,
446-
scoreboard.active_max,
447-
scoreboard.max_children_reached,
448-
scoreboard.slow_rq);
427+
(unsigned long) (now_epoch - scoreboard_p->start_epoch),
428+
scoreboard_p->requests,
429+
scoreboard_p->lq,
430+
scoreboard_p->lq_max,
431+
scoreboard_p->lq_len,
432+
scoreboard_p->idle,
433+
scoreboard_p->active,
434+
scoreboard_p->idle + scoreboard_p->active,
435+
scoreboard_p->active_max,
436+
scoreboard_p->max_children_reached,
437+
scoreboard_p->slow_rq);
449438

450439
PUTS(buffer);
451440
efree(buffer);
@@ -475,7 +464,7 @@ int fpm_status_handle_request(void) /* {{{ */
475464
if (!scoreboard_p->procs[i].used) {
476465
continue;
477466
}
478-
proc = scoreboard_p->procs[i];
467+
proc = &scoreboard_p->procs[i];
479468

480469
if (first) {
481470
first = 0;
@@ -487,44 +476,44 @@ int fpm_status_handle_request(void) /* {{{ */
487476

488477
query_string = NULL;
489478
tmp_query_string = NULL;
490-
if (proc.query_string[0] != '\0') {
479+
if (proc->query_string[0] != '\0') {
491480
if (!encode) {
492-
query_string = proc.query_string;
481+
query_string = proc->query_string;
493482
} else {
494-
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);
483+
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);
495484
query_string = ZSTR_VAL(tmp_query_string);
496485
}
497486
}
498487

499488
/* prevent NaN */
500-
if (proc.cpu_duration.tv_sec == 0 && proc.cpu_duration.tv_usec == 0) {
489+
if (proc->cpu_duration.tv_sec == 0 && proc->cpu_duration.tv_usec == 0) {
501490
cpu = 0.;
502491
} else {
503-
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.;
492+
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.;
504493
}
505494

506-
if (proc.request_stage == FPM_REQUEST_ACCEPTING) {
507-
duration = proc.duration;
495+
if (proc->request_stage == FPM_REQUEST_ACCEPTING) {
496+
duration = proc->duration;
508497
} else {
509-
timersub(&now, &proc.accepted, &duration);
498+
timersub(&now, &proc->accepted, &duration);
510499
}
511-
strftime(time_buffer, sizeof(time_buffer) - 1, time_format, localtime(&proc.start_epoch));
500+
strftime(time_buffer, sizeof(time_buffer) - 1, time_format, localtime(&proc->start_epoch));
512501
spprintf(&buffer, 0, full_syntax,
513-
(int) proc.pid,
514-
fpm_request_get_stage_name(proc.request_stage),
502+
(int) proc->pid,
503+
fpm_request_get_stage_name(proc->request_stage),
515504
time_buffer,
516-
(unsigned long) (now_epoch - proc.start_epoch),
517-
proc.requests,
505+
(unsigned long) (now_epoch - proc->start_epoch),
506+
proc->requests,
518507
duration.tv_sec * 1000000UL + duration.tv_usec,
519-
proc.request_method[0] != '\0' ? proc.request_method : "-",
520-
proc.request_uri[0] != '\0' ? proc.request_uri : "-",
508+
proc->request_method[0] != '\0' ? proc->request_method : "-",
509+
proc->request_uri[0] != '\0' ? proc->request_uri : "-",
521510
query_string ? "?" : "",
522511
query_string ? query_string : "",
523-
proc.content_length,
524-
proc.auth_user[0] != '\0' ? proc.auth_user : "-",
525-
proc.script_filename[0] != '\0' ? proc.script_filename : "-",
526-
proc.request_stage == FPM_REQUEST_ACCEPTING ? cpu : 0.,
527-
proc.request_stage == FPM_REQUEST_ACCEPTING ? proc.memory : 0);
512+
proc->content_length,
513+
proc->auth_user[0] != '\0' ? proc->auth_user : "-",
514+
proc->script_filename[0] != '\0' ? proc->script_filename : "-",
515+
proc->request_stage == FPM_REQUEST_ACCEPTING ? cpu : 0.,
516+
proc->request_stage == FPM_REQUEST_ACCEPTING ? proc->memory : 0);
528517
PUTS(buffer);
529518
efree(buffer);
530519

@@ -538,6 +527,7 @@ int fpm_status_handle_request(void) /* {{{ */
538527
}
539528
}
540529

530+
fpm_scoreboard_free_copy(scoreboard_p);
541531
return 1;
542532
}
543533

0 commit comments

Comments
 (0)