From 779b1131c56539ed74ab9db9a03f7bff71e203ba Mon Sep 17 00:00:00 2001 From: Max Romanov Date: Wed, 28 Oct 2020 00:01:46 +0300 Subject: Router: closing app worker's ports. --- src/nxt_router.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) (limited to 'src/nxt_router.c') diff --git a/src/nxt_router.c b/src/nxt_router.c index a3218047..bbd9a54e 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -4667,6 +4667,25 @@ nxt_router_free_app(nxt_task_t *task, void *obj, void *data) nxt_port_use(task, port, -1); } + nxt_thread_mutex_lock(&app->mutex); + + for ( ;; ) { + port = nxt_port_hash_retrieve(&app->port_hash); + if (port == NULL) { + break; + } + + app->port_hash_count--; + + port->app = NULL; + + nxt_port_close(task, port); + + nxt_port_use(task, port, -1); + } + + nxt_thread_mutex_unlock(&app->mutex); + nxt_assert(app->processes == 0); nxt_assert(app->active_requests == 0); nxt_assert(app->port_hash_count == 0); -- cgit From 38a9027fe54edcfc8157174a8ce575ed7acefa79 Mon Sep 17 00:00:00 2001 From: Max Romanov Date: Wed, 28 Oct 2020 00:01:46 +0300 Subject: Router: checking a buffer before accessing its memory fields. This fixes the router's crash on buildbot; the reason was an unexpected 'last' response from the application to the router arriving before the response headers. The last buffer is not a memory buffer, so the result of accessing memory fields is unpredictable. The unexpected 'last' message was caused by an error in libunit; fixed in fee8fd855a00. --- src/nxt_router.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/nxt_router.c') diff --git a/src/nxt_router.c b/src/nxt_router.c index bbd9a54e..6277bd47 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -3787,7 +3787,7 @@ nxt_router_response_ready_handler(nxt_task_t *task, nxt_port_recv_msg_t *msg, nxt_http_request_send_body(task, r, NULL); } else { - size_t b_size = nxt_buf_mem_used_size(&b->mem); + size_t b_size = nxt_buf_is_mem(b) ? nxt_buf_mem_used_size(&b->mem) : 0; if (nxt_slow_path(b_size < sizeof(*resp))) { goto fail; -- cgit From 735bb2f1276a7d768cffd1e780114a10018980cb Mon Sep 17 00:00:00 2001 From: Max Romanov Date: Wed, 28 Oct 2020 00:01:46 +0300 Subject: Added error response logging. Every internal server error response should have a clear description in log. --- src/nxt_router.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) (limited to 'src/nxt_router.c') diff --git a/src/nxt_router.c b/src/nxt_router.c index 6277bd47..15706428 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -3722,6 +3722,7 @@ static void nxt_router_response_ready_handler(nxt_task_t *task, nxt_port_recv_msg_t *msg, void *data) { + size_t b_size, count; nxt_int_t ret; nxt_app_t *app; nxt_buf_t *b, *next; @@ -3787,15 +3788,20 @@ nxt_router_response_ready_handler(nxt_task_t *task, nxt_port_recv_msg_t *msg, nxt_http_request_send_body(task, r, NULL); } else { - size_t b_size = nxt_buf_is_mem(b) ? nxt_buf_mem_used_size(&b->mem) : 0; + b_size = nxt_buf_is_mem(b) ? nxt_buf_mem_used_size(&b->mem) : 0; - if (nxt_slow_path(b_size < sizeof(*resp))) { + if (nxt_slow_path(b_size < sizeof(nxt_unit_response_t))) { + nxt_alert(task, "response buffer too small: %z", b_size); goto fail; } resp = (void *) b->mem.pos; - if (nxt_slow_path(b_size < sizeof(*resp) - + resp->fields_count * sizeof(nxt_unit_field_t))) { + count = (b_size - sizeof(nxt_unit_response_t)) + / sizeof(nxt_unit_field_t); + + if (nxt_slow_path(count < resp->fields_count)) { + nxt_alert(task, "response buffer too small for fields count: %D", + resp->fields_count); goto fail; } -- cgit From ccee391ab28d2742a98c612c42a37d6dcdbcd5f7 Mon Sep 17 00:00:00 2001 From: Max Romanov Date: Wed, 28 Oct 2020 00:01:46 +0300 Subject: Router: broadcasting the SHM_ACK message to all process ports. --- src/nxt_router.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'src/nxt_router.c') diff --git a/src/nxt_router.c b/src/nxt_router.c index 15706428..cf627746 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -5389,8 +5389,7 @@ nxt_router_oosm_handler(nxt_task_t *task, nxt_port_recv_msg_t *msg) nxt_thread_mutex_unlock(&process->incoming.mutex); if (ack) { - (void) nxt_port_socket_write(task, msg->port, NXT_PORT_MSG_SHM_ACK, - -1, 0, 0, NULL); + nxt_process_broadcast_shm_ack(task, process); } } -- cgit From 4cb8aeb31a8cf47f6c61aaccb95bbbf47cbc2393 Mon Sep 17 00:00:00 2001 From: Max Romanov Date: Wed, 28 Oct 2020 00:01:46 +0300 Subject: Router: introducing the PORT_ACK message. The PORT_ACK message is the router's response to the application's NEW_PORT message. After receiving PORT_ACK, the application is safe to process requests using this port. This message avoids a racing condition when the application starts processing a request from the shared queue and sends REQ_HEADERS_ACK. The REQ_HEADERS_ACK message contains the application port ID as reply_port, which the router uses to send request data. When the application creates a new port, it immediately sends it to the main router thread. Because the request is processed outside the main thread, a racing condition can occur between the receipt of the new port in the main thread and the receipt of REQ_HEADERS_ACK in the worker router thread where the same port is specified as reply_port. --- src/nxt_router.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/nxt_router.c') diff --git a/src/nxt_router.c b/src/nxt_router.c index cf627746..fbc9a4c8 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -688,6 +688,8 @@ nxt_router_new_port_handler(nxt_task_t *task, nxt_port_recv_msg_t *msg) port->app = app; port->main_app_port = main_app_port; + + nxt_port_socket_write(task, port, NXT_PORT_MSG_PORT_ACK, -1, 0, 0, NULL); } -- cgit From 896d8e8bfb3d8649db467d92e06c789b789d3feb Mon Sep 17 00:00:00 2001 From: Max Romanov Date: Tue, 10 Nov 2020 22:27:08 +0300 Subject: Fixing multi-buffer body send to application. Application shared queue only capable to pass one shared memory buffer. The rest buffers in chain needs to be send directly to application in response to REQ_HEADERS_AC message. The issue can be reproduced for configurations where 'body_buffer_size' is greater than memory segment size (10 Mb). Requests with body size greater than 10 Mb are just `stuck` e.g. not passed to application awaiting for more data from router. The bug was introduced in 1d84b9e4b459 (v1.19.0). --- src/nxt_router.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) (limited to 'src/nxt_router.c') diff --git a/src/nxt_router.c b/src/nxt_router.c index fbc9a4c8..95c8c160 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -3912,6 +3912,7 @@ nxt_router_req_headers_ack_handler(nxt_task_t *task, { int res; nxt_app_t *app; + nxt_buf_t *b; nxt_bool_t start_process, unlinked; nxt_port_t *app_port, *main_app_port, *idle_port; nxt_queue_link_t *idle_lnk; @@ -4009,16 +4010,25 @@ nxt_router_req_headers_ack_handler(nxt_task_t *task, req_rpc_data->app_port = app_port; - if (req_rpc_data->msg_info.body_fd != -1) { + b = req_rpc_data->msg_info.buf; + + if (b != NULL) { + /* First buffer is already sent. Start from second. */ + b = b->next; + } + + if (req_rpc_data->msg_info.body_fd != -1 || b != NULL) { nxt_debug(task, "stream #%uD: send body fd %d", req_rpc_data->stream, req_rpc_data->msg_info.body_fd); - lseek(req_rpc_data->msg_info.body_fd, 0, SEEK_SET); + if (req_rpc_data->msg_info.body_fd != -1) { + lseek(req_rpc_data->msg_info.body_fd, 0, SEEK_SET); + } res = nxt_port_socket_write(task, app_port, NXT_PORT_MSG_REQ_BODY, req_rpc_data->msg_info.body_fd, req_rpc_data->stream, - task->thread->engine->port->id, NULL); + task->thread->engine->port->id, b); if (nxt_slow_path(res != NXT_OK)) { nxt_http_request_error(task, r, NXT_HTTP_INTERNAL_SERVER_ERROR); -- cgit From fb80502513bf0140c5e595714967f75ea3e1e5d3 Mon Sep 17 00:00:00 2001 From: Valentin Bartenev Date: Tue, 17 Nov 2020 16:50:06 +0300 Subject: HTTP parser: allowed more characters in header field names. Previously, all requests that contained in header field names characters other than alphanumeric, or "-", or "_" were rejected with a 400 "Bad Request" error response. Now, the parser allows the same set of characters as specified in RFC 7230, including: "!", "#", "$", "%", "&", "'", "*", "+", ".", "^", "`", "|", and "~". Header field names that contain only these characters are considered valid. Also, there's a new option introduced: "discard_unsafe_fields". It accepts boolean value and it is set to "true" by default. When this option is "true", all header field names that contain characters in valid range, but other than alphanumeric or "-" are skipped during parsing. When the option is "false", these header fields aren't skipped. Requests with non-valid characters in header field names according to RFC 7230 are rejected regardless of "discard_unsafe_fields" setting. This closes #422 issue on GitHub. --- src/nxt_router.c | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'src/nxt_router.c') diff --git a/src/nxt_router.c b/src/nxt_router.c index 95c8c160..9dd5c30e 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -1262,6 +1262,12 @@ static nxt_conf_map_t nxt_router_http_conf[] = { NXT_CONF_MAP_STR, offsetof(nxt_socket_conf_t, body_temp_path), }, + + { + nxt_string("discard_unsafe_fields"), + NXT_CONF_MAP_INT8, + offsetof(nxt_socket_conf_t, discard_unsafe_fields), + }, }; @@ -1649,6 +1655,7 @@ nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf, skcf->header_buffer_size = 2048; skcf->large_header_buffer_size = 8192; skcf->large_header_buffers = 4; + skcf->discard_unsafe_fields = 1; skcf->body_buffer_size = 16 * 1024; skcf->max_body_size = 8 * 1024 * 1024; skcf->proxy_header_buffer_size = 64 * 1024; -- cgit