From 160eef0a0cc87d8550f7b53820d72931da499164 Mon Sep 17 00:00:00 2001 From: "William A. Rowe Jr" Date: Wed, 27 Apr 2016 18:41:49 +0000 Subject: [PATCH] Ensure http2 follows http in the meaning of status WRITE (meaning 'in the request processing phase' even if still consuming the request body, not literally in a 'now writing' state). Ensure a number of MPMs and the h2 connection io no longer clobber the request status line during state-only changes. While at it, clean up some very ugly formatting and unnecessary decoration, and avoid the wordy _from_conn() flavor when we are not passing a connection_rec. Ensure the useragent_ip is only used in the case where it has been initialized, fall back on the connection's remote_ip if the status is accidently updated from an uninitialized request_rec. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1741310 13f79535-47bb-0310-9956-ffa450edef68 --- modules/http2/h2_conn_io.c | 2 +- modules/http2/h2_task.c | 2 +- server/core.c | 2 +- server/mpm/event/event.c | 15 +++++++-------- server/mpm/motorz/motorz.c | 16 +++++++--------- server/mpm/simple/simple_io.c | 2 +- server/mpm/winnt/child.c | 11 +++++------ server/mpm/worker/worker.c | 13 ++++++++----- server/scoreboard.c | 2 +- 9 files changed, 32 insertions(+), 33 deletions(-) diff --git a/modules/http2/h2_conn_io.c b/modules/http2/h2_conn_io.c index badee77424..2e0a5f2d69 100644 --- a/modules/http2/h2_conn_io.c +++ b/modules/http2/h2_conn_io.c @@ -191,7 +191,7 @@ static apr_status_t pass_out(apr_bucket_brigade *bb, void *ctx) return APR_SUCCESS; } - ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_WRITE, c); + ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, NULL); apr_brigade_length(bb, 0, &bblen); h2_conn_io_bb_log(c, 0, APLOG_TRACE2, "master conn pass", bb); status = ap_pass_brigade(c->output_filters, bb); diff --git a/modules/http2/h2_task.c b/modules/http2/h2_task.c index f9b21326d4..968f7ffb2a 100644 --- a/modules/http2/h2_task.c +++ b/modules/http2/h2_task.c @@ -651,7 +651,7 @@ static apr_status_t h2_task_process_request(h2_task *task, conn_rec *c) "h2_task(%s): create request_rec", task->id); r = h2_request_create_rec(req, c); if (r && (r->status == HTTP_OK)) { - ap_update_child_status(c->sbh, SERVER_BUSY_READ, r); + ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r); if (cs) { cs->state = CONN_STATE_HANDLER; diff --git a/server/core.c b/server/core.c index 8540d225f9..5db957c394 100644 --- a/server/core.c +++ b/server/core.c @@ -5166,7 +5166,7 @@ static conn_rec *core_create_conn(apr_pool_t *ptrans, server_rec *s, core_server_config *sconf = ap_get_core_module_config(s->module_config); c->sbh = sbh; - (void)ap_update_child_status(c->sbh, SERVER_BUSY_READ, (request_rec *)NULL); + ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL); /* Got a connection structure, so initialize what fields we can * (the rest are zeroed out by pcalloc). diff --git a/server/mpm/event/event.c b/server/mpm/event/event.c index 885ea5a05e..58ff4307fc 100644 --- a/server/mpm/event/event.c +++ b/server/mpm/event/event.c @@ -1158,7 +1158,7 @@ read_request: if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) { int not_complete_yet; - ap_update_child_status_from_conn(sbh, SERVER_BUSY_WRITE, c); + ap_update_child_status(sbh, SERVER_BUSY_WRITE, NULL); not_complete_yet = ap_run_output_pending(c); @@ -1199,7 +1199,7 @@ read_request: start_lingering_close_blocking(cs); } else if (cs->pub.state == CONN_STATE_CHECK_REQUEST_LINE_READABLE) { - ap_update_child_status_from_conn(sbh, SERVER_BUSY_KEEPALIVE, c); + ap_update_child_status(sbh, SERVER_BUSY_KEEPALIVE, NULL); /* It greatly simplifies the logic to use a single timeout value per q * because the new element can just be added to the end of the list and @@ -2224,7 +2224,8 @@ static void *APR_THREAD_FUNC worker_thread(apr_thread_t * thd, void *dummy) } ap_update_child_status_from_indexes(process_slot, thread_slot, - dying ? SERVER_GRACEFUL : SERVER_READY, NULL); + dying ? SERVER_GRACEFUL + : SERVER_READY, NULL); worker_pop: if (workers_may_exit) { break; @@ -2280,9 +2281,8 @@ static void *APR_THREAD_FUNC worker_thread(apr_thread_t * thd, void *dummy) } ap_update_child_status_from_indexes(process_slot, thread_slot, - dying ? SERVER_DEAD : - SERVER_GRACEFUL, - (request_rec *) NULL); + dying ? SERVER_DEAD + : SERVER_GRACEFUL, NULL); apr_thread_exit(thd, APR_SUCCESS); return NULL; @@ -3044,8 +3044,7 @@ static void server_main_loop(int remaining_children_to_start, int num_buckets) for (i = 0; i < threads_per_child; i++) ap_update_child_status_from_indexes(child_slot, i, - SERVER_DEAD, - (request_rec *) NULL); + SERVER_DEAD, NULL); event_note_child_killed(child_slot, 0, 0); ps = &ap_scoreboard_image->parent[child_slot]; diff --git a/server/mpm/motorz/motorz.c b/server/mpm/motorz/motorz.c index c1f90c37ea..f048e246d7 100644 --- a/server/mpm/motorz/motorz.c +++ b/server/mpm/motorz/motorz.c @@ -400,7 +400,7 @@ read_request: ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf, APLOGNO(03331) "motorz_io_process(): CONN_STATE_WRITE_COMPLETION"); - ap_update_child_status_from_conn(scon->sbh, SERVER_BUSY_WRITE, c); + ap_update_child_status(scon->sbh, SERVER_BUSY_WRITE, NULL); not_complete_yet = ap_run_output_pending(c); @@ -928,7 +928,7 @@ static void child_main(motorz_core_t *mz, int child_num_arg, int child_bucket) ap_create_sb_handle(&sbh, pchild, my_child_num, 0); - (void) ap_update_child_status(sbh, SERVER_READY, (request_rec *) NULL); + ap_update_child_status(sbh, SERVER_READY, NULL); apr_skiplist_init(&mz->timeout_ring, mz->pool); apr_skiplist_set_compare(mz->timeout_ring, timer_comp, timer_comp); @@ -1001,7 +1001,7 @@ static void child_main(motorz_core_t *mz, int child_num_arg, int child_bucket) clean_child_exit(0); } - (void) ap_update_child_status(sbh, SERVER_READY, (request_rec *) NULL); + ap_update_child_status(sbh, SERVER_READY, NULL); { apr_time_t tnow = apr_time_now(); motorz_timer_t *te; @@ -1086,8 +1086,7 @@ static int make_child(motorz_core_t *mz, server_rec *s, int slot, int bucket) return -1; } - (void) ap_update_child_status_from_indexes(slot, 0, SERVER_STARTING, - (request_rec *) NULL); + ap_update_child_status_from_indexes(slot, 0, SERVER_STARTING, NULL); if ((pid = fork()) == -1) { ap_log_error(APLOG_MARK, APLOG_ERR, errno, s, APLOGNO(02872) "fork: Unable to fork new process"); @@ -1095,8 +1094,7 @@ static int make_child(motorz_core_t *mz, server_rec *s, int slot, int bucket) /* fork didn't succeed. Fix the scoreboard or else * it will say SERVER_STARTING forever and ever */ - (void) ap_update_child_status_from_indexes(slot, 0, SERVER_DEAD, - (request_rec *) NULL); + ap_update_child_status_from_indexes(slot, 0, SERVER_DEAD, NULL); /* In case system resources are maxxed out, we don't want * Apache running away with the CPU trying to fork over and @@ -1316,8 +1314,8 @@ static int motorz_run(apr_pool_t *_pconf, apr_pool_t *plog, server_rec *s) /* non-fatal death... note that it's gone in the scoreboard. */ if (child_slot >= 0) { - (void) ap_update_child_status_from_indexes(child_slot, 0, SERVER_DEAD, - (request_rec *) NULL); + ap_update_child_status_from_indexes(child_slot, 0, + SERVER_DEAD, NULL); motorz_note_child_killed(child_slot, 0, 0); if (remaining_children_to_start && child_slot < ap_num_kids) { diff --git a/server/mpm/simple/simple_io.c b/server/mpm/simple/simple_io.c index d06642cb49..3526eba10f 100644 --- a/server/mpm/simple/simple_io.c +++ b/server/mpm/simple/simple_io.c @@ -95,7 +95,7 @@ static apr_status_t simple_io_process(simple_conn_t * scon) if (scon->cs.state == CONN_STATE_WRITE_COMPLETION) { int not_complete_yet; - ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_WRITE, c); + ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, NULL); not_complete_yet = ap_run_output_pending(c); if (not_complete_yet > OK) { diff --git a/server/mpm/winnt/child.c b/server/mpm/winnt/child.c index 34aa1080db..e9a7190516 100644 --- a/server/mpm/winnt/child.c +++ b/server/mpm/winnt/child.c @@ -894,8 +894,7 @@ static DWORD __stdcall worker_main(void *thread_num_val) } } - ap_update_child_status_from_indexes(0, thread_num, SERVER_DEAD, - (request_rec *) NULL); + ap_update_child_status_from_indexes(0, thread_num, SERVER_DEAD, NULL); return 0; } @@ -1327,13 +1326,13 @@ void child_main(apr_pool_t *pconf, DWORD parent_pid) threads_created); } for (i = 0; i < threads_created; i++) { - int *score_idx; + int *idx; TerminateThread(child_handles[i], 1); CloseHandle(child_handles[i]); /* Reset the scoreboard entry for the thread we just whacked */ - score_idx = apr_hash_get(ht, &child_handles[i], sizeof(HANDLE)); - if (score_idx) { - ap_update_child_status_from_indexes(0, *score_idx, SERVER_DEAD, NULL); + idx = apr_hash_get(ht, &child_handles[i], sizeof(HANDLE)); + if (idx) { + ap_update_child_status_from_indexes(0, *idx, SERVER_DEAD, NULL); } } ap_log_error(APLOG_MARK, APLOG_NOTICE, APR_SUCCESS, ap_server_conf, APLOGNO(00364) diff --git a/server/mpm/worker/worker.c b/server/mpm/worker/worker.c index 045c57545c..9c49d832cc 100644 --- a/server/mpm/worker/worker.c +++ b/server/mpm/worker/worker.c @@ -930,7 +930,8 @@ static void * APR_THREAD_FUNC worker_thread(apr_thread_t *thd, void * dummy) ap_scoreboard_image->servers[process_slot][thread_slot].pid = ap_my_pid; ap_scoreboard_image->servers[process_slot][thread_slot].tid = apr_os_thread_current(); ap_scoreboard_image->servers[process_slot][thread_slot].generation = retained->my_generation; - ap_update_child_status_from_indexes(process_slot, thread_slot, SERVER_STARTING, NULL); + ap_update_child_status_from_indexes(process_slot, thread_slot, + SERVER_STARTING, NULL); #ifdef HAVE_PTHREAD_KILL unblock_signal(WORKER_SIGNAL); @@ -951,7 +952,8 @@ static void * APR_THREAD_FUNC worker_thread(apr_thread_t *thd, void * dummy) is_idle = 1; } - ap_update_child_status_from_indexes(process_slot, thread_slot, SERVER_READY, NULL); + ap_update_child_status_from_indexes(process_slot, thread_slot, + SERVER_READY, NULL); worker_pop: if (workers_may_exit) { break; @@ -997,7 +999,8 @@ worker_pop: } ap_update_child_status_from_indexes(process_slot, thread_slot, - (dying) ? SERVER_DEAD : SERVER_GRACEFUL, (request_rec *) NULL); + dying ? SERVER_DEAD + : SERVER_GRACEFUL, NULL); apr_thread_exit(thd, APR_SUCCESS); return NULL; @@ -1734,8 +1737,8 @@ static void server_main_loop(int remaining_children_to_start, int num_buckets) process_score *ps; for (i = 0; i < threads_per_child; i++) - ap_update_child_status_from_indexes(child_slot, i, SERVER_DEAD, - (request_rec *) NULL); + ap_update_child_status_from_indexes(child_slot, i, + SERVER_DEAD, NULL); worker_note_child_killed(child_slot, 0, 0); ps = &ap_scoreboard_image->parent[child_slot]; diff --git a/server/scoreboard.c b/server/scoreboard.c index 4e79a1f714..10128c0334 100644 --- a/server/scoreboard.c +++ b/server/scoreboard.c @@ -501,7 +501,7 @@ static int update_child_status_internal(int child_num, copy_request(ws->request, sizeof(ws->request), r); } - if (r) { + if (r && r->useragent_ip) { if (!(val = ap_get_useragent_host(r, REMOTE_NOLOOKUP, NULL))) apr_cpystrn(ws->client, r->useragent_ip, sizeof(ws->client)); else -- 2.50.1