From c2f7f2964c9394643b69076ab8f0df490b000f45 Mon Sep 17 00:00:00 2001 From: Andrew Clayton Date: Fri, 23 Feb 2024 01:05:04 +0000 Subject: Avoid potential NULL pointer dereference in nxt_router_temp_conf() In nxt_router_temp_conf() we have rtcf = nxt_mp_zget(mp, sizeof(nxt_router_conf_t)); if (nxt_slow_path(rtcf == NULL)) { goto fail; } If rtcf is NULL then we do fail: if (rtcf->tstr_state != NULL) { nxt_tstr_state_release(rtcf->tstr_state); } In which case we will dereference the NULL pointer rtcf. This patch re-works the goto labels to make them more specific to their intended purpose and ensures we are freeing things which have been allocated. This was found by the clang static analyser. Reviewed-by: Zhidao Hong Signed-off-by: Andrew Clayton --- src/nxt_router.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) (limited to 'src/nxt_router.c') diff --git a/src/nxt_router.c b/src/nxt_router.c index 1a1aca2b..e395929e 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -1077,14 +1077,14 @@ nxt_router_temp_conf(nxt_task_t *task) rtcf = nxt_mp_zget(mp, sizeof(nxt_router_conf_t)); if (nxt_slow_path(rtcf == NULL)) { - goto fail; + goto out_free_mp; } rtcf->mem_pool = mp; rtcf->tstr_state = nxt_tstr_state_new(mp, 0); if (nxt_slow_path(rtcf->tstr_state == NULL)) { - goto fail; + goto out_free_mp; } #if (NXT_HAVE_NJS) @@ -1093,12 +1093,12 @@ nxt_router_temp_conf(nxt_task_t *task) tmp = nxt_mp_create(1024, 128, 256, 32); if (nxt_slow_path(tmp == NULL)) { - goto fail; + goto out_free_tstr_state; } tmcf = nxt_mp_zget(tmp, sizeof(nxt_router_temp_conf_t)); if (nxt_slow_path(tmcf == NULL)) { - goto temp_fail; + goto out_free; } tmcf->mem_pool = tmp; @@ -1109,7 +1109,7 @@ nxt_router_temp_conf(nxt_task_t *task) tmcf->engines = nxt_array_create(tmcf->mem_pool, 4, sizeof(nxt_router_engine_conf_t)); if (nxt_slow_path(tmcf->engines == NULL)) { - goto temp_fail; + goto out_free; } nxt_queue_init(&creating_sockets); @@ -1131,16 +1131,18 @@ nxt_router_temp_conf(nxt_task_t *task) return tmcf; -temp_fail: +out_free: nxt_mp_destroy(tmp); -fail: +out_free_tstr_state: if (rtcf->tstr_state != NULL) { nxt_tstr_state_release(rtcf->tstr_state); } +out_free_mp: + nxt_mp_destroy(mp); return NULL; -- cgit From 8f861cf4d15e8befca6edcee4b04b5304f082f05 Mon Sep 17 00:00:00 2001 From: Andrew Clayton Date: Tue, 16 Apr 2024 20:30:48 +0100 Subject: Constify a bunch of static local variables A common pattern was to declare variables in functions like static nxt_str_t ... Not sure why static, as they were being treated more like string literals (and of course they are _not_ thread safe), let's actually make them constants (qualifier wise). This handles core code conversion. Reviewed-by: Zhidao HONG Signed-off-by: Andrew Clayton --- src/nxt_router.c | 46 +++++++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 21 deletions(-) (limited to 'src/nxt_router.c') diff --git a/src/nxt_router.c b/src/nxt_router.c index e395929e..48870d20 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -1634,25 +1634,29 @@ nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf, nxt_router_app_conf_t apcf; nxt_router_listener_conf_t lscf; - static nxt_str_t http_path = nxt_string("/settings/http"); - static nxt_str_t applications_path = nxt_string("/applications"); - static nxt_str_t listeners_path = nxt_string("/listeners"); - static nxt_str_t routes_path = nxt_string("/routes"); - static nxt_str_t access_log_path = nxt_string("/access_log"); + static const nxt_str_t http_path = nxt_string("/settings/http"); + static const nxt_str_t applications_path = nxt_string("/applications"); + static const nxt_str_t listeners_path = nxt_string("/listeners"); + static const nxt_str_t routes_path = nxt_string("/routes"); + static const nxt_str_t access_log_path = nxt_string("/access_log"); #if (NXT_TLS) - static nxt_str_t certificate_path = nxt_string("/tls/certificate"); - static nxt_str_t conf_commands_path = nxt_string("/tls/conf_commands"); - static nxt_str_t conf_cache_path = nxt_string("/tls/session/cache_size"); - static nxt_str_t conf_timeout_path = nxt_string("/tls/session/timeout"); - static nxt_str_t conf_tickets = nxt_string("/tls/session/tickets"); + static const nxt_str_t certificate_path = nxt_string("/tls/certificate"); + static const nxt_str_t conf_commands_path = + nxt_string("/tls/conf_commands"); + static const nxt_str_t conf_cache_path = + nxt_string("/tls/session/cache_size"); + static const nxt_str_t conf_timeout_path = + nxt_string("/tls/session/timeout"); + static const nxt_str_t conf_tickets = nxt_string("/tls/session/tickets"); #endif #if (NXT_HAVE_NJS) - static nxt_str_t js_module_path = nxt_string("/settings/js_module"); + static const nxt_str_t js_module_path = nxt_string("/settings/js_module"); #endif - static nxt_str_t static_path = nxt_string("/settings/http/static"); - static nxt_str_t websocket_path = nxt_string("/settings/http/websocket"); - static nxt_str_t forwarded_path = nxt_string("/forwarded"); - static nxt_str_t client_ip_path = nxt_string("/client_ip"); + static const nxt_str_t static_path = nxt_string("/settings/http/static"); + static const nxt_str_t websocket_path = + nxt_string("/settings/http/websocket"); + static const nxt_str_t forwarded_path = nxt_string("/forwarded"); + static const nxt_str_t client_ip_path = nxt_string("/client_ip"); root = nxt_conf_json_parse(tmcf->mem_pool, start, end, NULL); if (root == NULL) { @@ -2283,7 +2287,7 @@ nxt_router_conf_process_static(nxt_task_t *task, nxt_router_conf_t *rtcf, nxt_uint_t exts; nxt_conf_value_t *mtypes_conf, *ext_conf, *value; - static nxt_str_t mtypes_path = nxt_string("/mime_types"); + static const nxt_str_t mtypes_path = nxt_string("/mime_types"); mp = rtcf->mem_pool; @@ -2360,11 +2364,11 @@ nxt_router_conf_forward(nxt_task_t *task, nxt_mp_t *mp, nxt_conf_value_t *conf) nxt_http_forward_t *forward; nxt_http_route_addr_rule_t *source; - static nxt_str_t header_path = nxt_string("/header"); - static nxt_str_t client_ip_path = nxt_string("/client_ip"); - static nxt_str_t protocol_path = nxt_string("/protocol"); - static nxt_str_t source_path = nxt_string("/source"); - static nxt_str_t recursive_path = nxt_string("/recursive"); + static const nxt_str_t header_path = nxt_string("/header"); + static const nxt_str_t client_ip_path = nxt_string("/client_ip"); + static const nxt_str_t protocol_path = nxt_string("/protocol"); + static const nxt_str_t source_path = nxt_string("/source"); + static const nxt_str_t recursive_path = nxt_string("/recursive"); header_conf = nxt_conf_get_path(conf, &header_path); -- cgit From 64f4c78bf441fa9e021d905a03d374d0a9e05e8d Mon Sep 17 00:00:00 2001 From: Zhidao HONG Date: Tue, 11 Jun 2024 17:59:00 +0800 Subject: http: Support chunked request bodies This is a temporary support for chunked request bodies by converting to Content-Length. This allows for processing of such requests until a more permanent solution is developed. A new configuration option "chunked_transform" has been added to enable this feature. The option can be set as follows: { "settings": { "chunked_transform": true } } By default, this option is set to false, which retains the current behaviour of rejecting chunked requests with a '411 Length Required' status code. Please note that this is an experimental implementation. Reviewed-by: Andrew Clayton --- 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 48870d20..43209451 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -1575,6 +1575,12 @@ static nxt_conf_map_t nxt_router_http_conf[] = { NXT_CONF_MAP_INT8, offsetof(nxt_socket_conf_t, server_version), }, + + { + nxt_string("chunked_transform"), + NXT_CONF_MAP_INT8, + offsetof(nxt_socket_conf_t, chunked_transform), + }, }; @@ -1994,6 +2000,7 @@ nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf, skcf->proxy_read_timeout = 30 * 1000; skcf->server_version = 1; + skcf->chunked_transform = 0; skcf->websocket_conf.max_frame_size = 1024 * 1024; skcf->websocket_conf.read_timeout = 60 * 1000; -- cgit From 57c88fd4086d47bf866e3fe7e4c03d0262d413ae Mon Sep 17 00:00:00 2001 From: Andrew Clayton Date: Mon, 12 Aug 2024 22:56:16 +0100 Subject: router: Make the number of router threads configurable Unit generally creates an extra number of router threads (to handle client connections, not incl the main thread) to match the number of available CPUs. There are cases when this can go wrong, e.g on a high CPU count machine and Unit is being effectively limited to a few CPUs via the cgroups cpu controller. So Unit may create a large number of router threads when they are only going to effectively run on a couple of CPUs or so. There may be other cases where you would like to tweak the number of router threads, depending on your workload. As it turns out it looks like it was intended to be made configurable but was just never hooked up to the config system. This adds a new '/settings/listen_threads' config option which can be set like { "listen": { ... }, "settings": { "listen_threads": 2, ... }, ... } Before this patch (on a four cpu system) $ ps -efL | grep router andrew 419832 419829 419832 0 5 Aug12 pts/10 00:00:00 unit: router andrew 419832 419829 419833 0 5 Aug12 pts/10 00:00:00 unit: router andrew 419832 419829 419834 0 5 Aug12 pts/10 00:00:00 unit: router andrew 419832 419829 445145 0 5 03:31 pts/10 00:00:00 unit: router andrew 419832 419829 445146 0 5 03:31 pts/10 00:00:00 unit: router After, with a threads setting of 2 $ ps -efL | grep router andrew 419832 419829 419832 0 3 Aug12 pts/10 00:00:00 unit: router andrew 419832 419829 419833 0 3 Aug12 pts/10 00:00:00 unit: router andrew 419832 419829 419834 0 3 Aug12 pts/10 00:00:00 unit: router Closes: https://github.com/nginx/unit/issues/1042 Signed-off-by: Andrew Clayton --- src/nxt_router.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) (limited to 'src/nxt_router.c') diff --git a/src/nxt_router.c b/src/nxt_router.c index 43209451..c8ba4744 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -1412,7 +1412,7 @@ nxt_router_conf_send(nxt_task_t *task, nxt_router_temp_conf_t *tmcf, static nxt_conf_map_t nxt_router_conf[] = { { - nxt_string("listeners_threads"), + nxt_string("listen_threads"), NXT_CONF_MAP_INT32, offsetof(nxt_router_conf_t, threads), }, @@ -1630,7 +1630,7 @@ nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf, nxt_conf_value_t *js_module; #endif nxt_conf_value_t *root, *conf, *http, *value, *websocket; - nxt_conf_value_t *applications, *application; + nxt_conf_value_t *applications, *application, *settings; nxt_conf_value_t *listeners, *listener; nxt_socket_conf_t *skcf; nxt_router_conf_t *rtcf; @@ -1640,6 +1640,7 @@ nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf, nxt_router_app_conf_t apcf; nxt_router_listener_conf_t lscf; + static const nxt_str_t settings_path = nxt_string("/settings"); static const nxt_str_t http_path = nxt_string("/settings/http"); static const nxt_str_t applications_path = nxt_string("/applications"); static const nxt_str_t listeners_path = nxt_string("/listeners"); @@ -1673,11 +1674,14 @@ nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf, rtcf = tmcf->router_conf; mp = rtcf->mem_pool; - ret = nxt_conf_map_object(mp, root, nxt_router_conf, - nxt_nitems(nxt_router_conf), rtcf); - if (ret != NXT_OK) { - nxt_alert(task, "root map error"); - return NXT_ERROR; + settings = nxt_conf_get_path(root, &settings_path); + if (settings != NULL) { + ret = nxt_conf_map_object(mp, settings, nxt_router_conf, + nxt_nitems(nxt_router_conf), rtcf); + if (ret != NXT_OK) { + nxt_alert(task, "router_conf map error"); + return NXT_ERROR; + } } if (rtcf->threads == 0) { -- cgit From 76489fb7e0ab9142651b91ee8072c6cda270dd09 Mon Sep 17 00:00:00 2001 From: Andrew Clayton Date: Wed, 14 Aug 2024 00:33:13 +0100 Subject: conf, router: Make the listen(2) backlog configurable @oopsoop2 on GitHub reported a performance issue related to the default listen(2) backlog size of 511 on nginx. They found that increasing it helped, nginx has a config option to configure this. They would like to be able to do the same on Unit (which also defaults to 511 on some systems). This seems reasonable. NOTE: On Linux before commit 97c15fa38 ("socket: Use a default listen backlog of -1 on Linux") we defaulted to 511. Since that commit we default to the Kernels default, which before 5.4 is 128 and after is 4096. This adds a new per-listener 'backlog' config option, e.g { "listeners": { "[::1]:8080": { "pass": "routes", "backlog": 1024 }, } ... } This doesn't effect the control socket. Closes: https://github.com/nginx/unit/issues/1384 Reported-by: Signed-off-by: Andrew Clayton --- src/nxt_router.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) (limited to 'src/nxt_router.c') diff --git a/src/nxt_router.c b/src/nxt_router.c index c8ba4744..076cd134 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -40,6 +40,7 @@ typedef struct { typedef struct { nxt_str_t pass; nxt_str_t application; + int backlog; } nxt_router_listener_conf_t; @@ -166,7 +167,7 @@ static void nxt_router_app_prefork_ready(nxt_task_t *task, static void nxt_router_app_prefork_error(nxt_task_t *task, nxt_port_recv_msg_t *msg, void *data); static nxt_socket_conf_t *nxt_router_socket_conf(nxt_task_t *task, - nxt_router_temp_conf_t *tmcf, nxt_str_t *name); + nxt_router_temp_conf_t *tmcf, nxt_str_t *name, int backlog); static nxt_int_t nxt_router_listen_socket_find(nxt_router_temp_conf_t *tmcf, nxt_socket_conf_t *nskcf, nxt_sockaddr_t *sa); @@ -1494,6 +1495,12 @@ static nxt_conf_map_t nxt_router_listener_conf[] = { NXT_CONF_MAP_STR_COPY, offsetof(nxt_router_listener_conf_t, application), }, + + { + nxt_string("backlog"), + NXT_CONF_MAP_INT32, + offsetof(nxt_router_listener_conf_t, backlog), + }, }; @@ -1968,13 +1975,10 @@ nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf, break; } - skcf = nxt_router_socket_conf(task, tmcf, &name); - if (skcf == NULL) { - goto fail; - } - nxt_memzero(&lscf, sizeof(lscf)); + lscf.backlog = -1; + ret = nxt_conf_map_object(mp, listener, nxt_router_listener_conf, nxt_nitems(nxt_router_listener_conf), &lscf); @@ -1985,6 +1989,11 @@ nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf, nxt_debug(task, "application: %V", &lscf.application); + skcf = nxt_router_socket_conf(task, tmcf, &name, lscf.backlog); + if (skcf == NULL) { + goto fail; + } + // STUB, default values if http block is not defined. skcf->header_buffer_size = 2048; skcf->large_header_buffer_size = 8192; @@ -2688,7 +2697,7 @@ nxt_router_application_init(nxt_router_conf_t *rtcf, nxt_str_t *name, static nxt_socket_conf_t * nxt_router_socket_conf(nxt_task_t *task, nxt_router_temp_conf_t *tmcf, - nxt_str_t *name) + nxt_str_t *name, int backlog) { size_t size; nxt_int_t ret; @@ -2732,7 +2741,7 @@ nxt_router_socket_conf(nxt_task_t *task, nxt_router_temp_conf_t *tmcf, nxt_listen_socket_remote_size(ls); ls->socket = -1; - ls->backlog = NXT_LISTEN_BACKLOG; + ls->backlog = backlog > -1 ? backlog : NXT_LISTEN_BACKLOG; ls->flags = NXT_NONBLOCK; ls->read_after_accept = 1; } @@ -2879,7 +2888,7 @@ nxt_router_listen_socket_ready(nxt_task_t *task, nxt_port_recv_msg_t *msg, nxt_socket_defer_accept(task, s, rpc->socket_conf->listen->sockaddr); - ret = nxt_listen_socket(task, s, NXT_LISTEN_BACKLOG); + ret = nxt_listen_socket(task, s, rpc->socket_conf->listen->backlog); if (nxt_slow_path(ret != NXT_OK)) { goto fail; } -- cgit