Skip to content

Commit 059f9f7

Browse files
bukkabukka
committed
Fix phpGH-19974: fpm_status_export_to_zval segfault for parallel execution
The fix fixes some other races that could result in mixed up copy and nprocs number. It requires creating a copy in a similar way like for request status. This was not previously used to not impact other requests. However this does not make that much sense as the only thing that impacts it is holding the lock and not waiting for it. It is true that if there was a big contention then the lock would be acquired more often but that can be achieved by using fpm_get_status in loop so it should not make a huge difference hopefully. Closes phpGH-19974
1 parent c5fa769 commit 059f9f7

File tree

2 files changed

+46
-59
lines changed

2 files changed

+46
-59
lines changed

‎NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ PHP NEWS
77
. Fixed bug GH-20073 (Assertion failure in WeakMap offset operations on
88
reference). (nielsdos)
99

10+
- FPM:
11+
. Fixed bug GH-19974 (fpm_status_export_to_zval segfault for parallel
12+
execution). (Jakub Zelenka, txuna)
13+
1014
- Random:
1115
. Fix Randomizer::__serialize() w.r.t. INDIRECTs. (nielsdos)
1216

‎sapi/fpm/fpm/fpm_status.c‎

Lines changed: 42 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -47,92 +47,75 @@ int fpm_status_init_child(struct fpm_worker_pool_s *wp) /* {{{ */
4747

4848
int fpm_status_export_to_zval(zval *status)
4949
{
50-
struct fpm_scoreboard_s scoreboard, *scoreboard_p;
50+
struct fpm_scoreboard_s *scoreboard_p;
51+
struct fpm_scoreboard_proc_s *proc_p;
5152
zval fpm_proc_stats, fpm_proc_stat;
5253
time_t now_epoch;
5354
struct timeval duration, now;
5455
double cpu;
5556
int i;
5657

57-
scoreboard_p = fpm_scoreboard_acquire(NULL, 1);
58-
if (!scoreboard_p) {
59-
zlog(ZLOG_NOTICE, "[pool (unknown)] status: scoreboard already in use.");
60-
return -1;
61-
}
62-
63-
/* copy the scoreboard not to bother other processes */
64-
scoreboard = *scoreboard_p;
65-
struct fpm_scoreboard_proc_s *procs = safe_emalloc(
66-
sizeof(struct fpm_scoreboard_proc_s), scoreboard.nprocs, 0);
67-
68-
struct fpm_scoreboard_proc_s *proc_p;
69-
for(i=0; i<scoreboard.nprocs; i++) {
70-
proc_p = fpm_scoreboard_proc_acquire(scoreboard_p, i, 1);
71-
if (!proc_p){
72-
procs[i].used=-1;
73-
continue;
74-
}
75-
procs[i] = *proc_p;
76-
fpm_scoreboard_proc_release(proc_p);
77-
}
78-
fpm_scoreboard_release(scoreboard_p);
58+
scoreboard_p = fpm_scoreboard_copy(NULL, 1);
7959

8060
now_epoch = time(NULL);
8161
fpm_clock_get(&now);
8262

8363
array_init(status);
84-
add_assoc_string(status, "pool", scoreboard.pool);
85-
add_assoc_string(status, "process-manager", PM2STR(scoreboard.pm));
86-
add_assoc_long(status, "start-time", scoreboard.start_epoch);
87-
add_assoc_long(status, "start-since", now_epoch - scoreboard.start_epoch);
88-
add_assoc_long(status, "accepted-conn", scoreboard.requests);
89-
add_assoc_long(status, "listen-queue", scoreboard.lq);
90-
add_assoc_long(status, "max-listen-queue", scoreboard.lq_max);
91-
add_assoc_long(status, "listen-queue-len", scoreboard.lq_len);
92-
add_assoc_long(status, "idle-processes", scoreboard.idle);
93-
add_assoc_long(status, "active-processes", scoreboard.active);
94-
add_assoc_long(status, "total-processes", scoreboard.idle + scoreboard.active);
95-
add_assoc_long(status, "max-active-processes", scoreboard.active_max);
96-
add_assoc_long(status, "max-children-reached", scoreboard.max_children_reached);
97-
add_assoc_long(status, "slow-requests", scoreboard.slow_rq);
64+
add_assoc_string(status, "pool", scoreboard_p->pool);
65+
add_assoc_string(status, "process-manager", PM2STR(scoreboard_p->pm));
66+
add_assoc_long(status, "start-time", scoreboard_p->start_epoch);
67+
add_assoc_long(status, "start-since", now_epoch - scoreboard_p->start_epoch);
68+
add_assoc_long(status, "accepted-conn", scoreboard_p->requests);
69+
add_assoc_long(status, "listen-queue", scoreboard_p->lq);
70+
add_assoc_long(status, "max-listen-queue", scoreboard_p->lq_max);
71+
add_assoc_long(status, "listen-queue-len", scoreboard_p->lq_len);
72+
add_assoc_long(status, "idle-processes", scoreboard_p->idle);
73+
add_assoc_long(status, "active-processes", scoreboard_p->active);
74+
add_assoc_long(status, "total-processes", scoreboard_p->idle + scoreboard_p->active);
75+
add_assoc_long(status, "max-active-processes", scoreboard_p->active_max);
76+
add_assoc_long(status, "max-children-reached", scoreboard_p->max_children_reached);
77+
add_assoc_long(status, "slow-requests", scoreboard_p->slow_rq);
9878

9979
array_init(&fpm_proc_stats);
100-
for(i=0; i<scoreboard.nprocs; i++) {
101-
if (!procs[i].used) {
80+
for(i = 0; i < scoreboard_p->nprocs; i++) {
81+
proc_p = &scoreboard_p->procs[i];
82+
if (!proc_p->used) {
10283
continue;
10384
}
104-
proc_p = &procs[i];
10585
/* prevent NaN */
106-
if (procs[i].cpu_duration.tv_sec == 0 && procs[i].cpu_duration.tv_usec == 0) {
86+
if (proc_p->cpu_duration.tv_sec == 0 && proc_p->cpu_duration.tv_usec == 0) {
10787
cpu = 0.;
10888
} else {
109-
cpu = (procs[i].last_request_cpu.tms_utime + procs[i].last_request_cpu.tms_stime + procs[i].last_request_cpu.tms_cutime + procs[i].last_request_cpu.tms_cstime) / fpm_scoreboard_get_tick() / (procs[i].cpu_duration.tv_sec + procs[i].cpu_duration.tv_usec / 1000000.) * 100.;
89+
cpu = (proc_p->last_request_cpu.tms_utime + proc_p->last_request_cpu.tms_stime + proc_p->last_request_cpu.tms_cutime +
90+
proc_p->last_request_cpu.tms_cstime) / fpm_scoreboard_get_tick() /
91+
(proc_p->cpu_duration.tv_sec + proc_p->cpu_duration.tv_usec / 1000000.) * 100.;
11092
}
11193

11294
array_init(&fpm_proc_stat);
113-
add_assoc_long(&fpm_proc_stat, "pid", procs[i].pid);
114-
add_assoc_string(&fpm_proc_stat, "state", fpm_request_get_stage_name(procs[i].request_stage));
115-
add_assoc_long(&fpm_proc_stat, "start-time", procs[i].start_epoch);
116-
add_assoc_long(&fpm_proc_stat, "start-since", now_epoch - procs[i].start_epoch);
117-
add_assoc_long(&fpm_proc_stat, "requests", procs[i].requests);
118-
if (procs[i].request_stage == FPM_REQUEST_ACCEPTING) {
119-
duration = procs[i].duration;
95+
add_assoc_long(&fpm_proc_stat, "pid", proc_p->pid);
96+
add_assoc_string(&fpm_proc_stat, "state", fpm_request_get_stage_name(proc_p->request_stage));
97+
add_assoc_long(&fpm_proc_stat, "start-time", proc_p->start_epoch);
98+
add_assoc_long(&fpm_proc_stat, "start-since", now_epoch - proc_p->start_epoch);
99+
add_assoc_long(&fpm_proc_stat, "requests", proc_p->requests);
100+
if (proc_p->request_stage == FPM_REQUEST_ACCEPTING) {
101+
duration = proc_p->duration;
120102
} else {
121-
timersub(&now, &procs[i].accepted, &duration);
103+
timersub(&now, &proc_p->accepted, &duration);
122104
}
123105
add_assoc_long(&fpm_proc_stat, "request-duration", duration.tv_sec * 1000000UL + duration.tv_usec);
124-
add_assoc_string(&fpm_proc_stat, "request-method", procs[i].request_method[0] != '\0' ? procs[i].request_method : "-");
125-
add_assoc_string(&fpm_proc_stat, "request-uri", procs[i].request_uri);
126-
add_assoc_string(&fpm_proc_stat, "query-string", procs[i].query_string);
127-
add_assoc_long(&fpm_proc_stat, "request-length", procs[i].content_length);
128-
add_assoc_string(&fpm_proc_stat, "user", procs[i].auth_user[0] != '\0' ? procs[i].auth_user : "-");
129-
add_assoc_string(&fpm_proc_stat, "script", procs[i].script_filename[0] != '\0' ? procs[i].script_filename : "-");
130-
add_assoc_double(&fpm_proc_stat, "last-request-cpu", procs[i].request_stage == FPM_REQUEST_ACCEPTING ? cpu : 0.);
131-
add_assoc_long(&fpm_proc_stat, "last-request-memory", procs[i].request_stage == FPM_REQUEST_ACCEPTING ? procs[i].memory : 0);
106+
add_assoc_string(&fpm_proc_stat, "request-method", proc_p->request_method[0] != '\0' ? proc_p->request_method : "-");
107+
add_assoc_string(&fpm_proc_stat, "request-uri", proc_p->request_uri);
108+
add_assoc_string(&fpm_proc_stat, "query-string", proc_p->query_string);
109+
add_assoc_long(&fpm_proc_stat, "request-length", proc_p->content_length);
110+
add_assoc_string(&fpm_proc_stat, "user", proc_p->auth_user[0] != '\0' ? proc_p->auth_user : "-");
111+
add_assoc_string(&fpm_proc_stat, "script", proc_p->script_filename[0] != '\0' ? proc_p->script_filename : "-");
112+
add_assoc_double(&fpm_proc_stat, "last-request-cpu", proc_p->request_stage == FPM_REQUEST_ACCEPTING ? cpu : 0.);
113+
add_assoc_long(&fpm_proc_stat, "last-request-memory", proc_p->request_stage == FPM_REQUEST_ACCEPTING ? proc_p->memory : 0);
132114
add_next_index_zval(&fpm_proc_stats, &fpm_proc_stat);
133115
}
134116
add_assoc_zval(status, "procs", &fpm_proc_stats);
135-
efree(procs);
117+
118+
fpm_scoreboard_free_copy(scoreboard_p);
136119

137120
return 0;
138121
}

0 commit comments

Comments
 (0)