From 33064cd077cf6fa386f0a5a840c2161868da7b3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 8 Feb 2022 12:42:34 +0100 Subject: [PATCH] Run .closehandle_cb asynchrounosly in nmhandle_detach_cb() When sock->closehandle_cb is set, we need to run nmhandle_detach_cb() asynchronously to ensure correct order of multiple packets processing in the isc__nm_process_sock_buffer(). When not run asynchronously, it would cause: a) out-of-order processing of the return codes from processbuffer(); b) stack growth because the next TCP DNS message read callback will be called from within the current TCP DNS message read callback. The sock->closehandle_cb is set to isc__nm_resume_processing() for TCP sockets which calls isc__nm_process_sock_buffer(). If the read callback (called from isc__nm_process_sock_buffer()->processbuffer()) doesn't attach to the nmhandle (f.e. because it wants to drop the processing or we send the response directly via uv_try_write()), the isc__nm_resume_processing() (via .closehandle_cb) would call isc__nm_process_sock_buffer() recursively. The below shortened code path shows how the stack can grow: 1: ns__client_request(handle, ...); 2: isc_nm_tcpdns_sequential(handle); 3: ns_query_start(client, handle); 4: query_lookup(qctx); 5: query_send(qctcx->client); 6: isc__nmhandle_detach(&client->reqhandle); 7: nmhandle_detach_cb(&handle); 8: sock->closehandle_cb(sock); // isc__nm_resume_processing 9: isc__nm_process_sock_buffer(sock); 10: processbuffer(sock); // isc__nm_tcpdns_processbuffer 11: isc_nmhandle_attach(req->handle, &handle); 12: isc__nm_readcb(sock, req, ISC_R_SUCCESS); 13: isc__nm_async_readcb(NULL, ...); 14: uvreq->cb.recv(...); // ns__client_request Instead, if 'sock->closehandle_cb' is set, we need to run detach the handle asynchroniously in 'isc__nmhandle_detach', so that on line 8 in the code flow above does not start this recursion. This ensures the correct order when processing multiple packets in the function 'isc__nm_process_sock_buffer()' and prevents the stack growth. When not run asynchronously, the out-of-order processing leaves the first TCP socket open until all requests on the stream have been processed. If the pipelining is disabled on the TCP via `keep-response-order` configuration option, named would keep the first socket in lingering CLOSE_WAIT state when the client sends an incomplete packet and then closes the connection from the client side. (cherry picked from commit afee2b5a7bc933a2d987907fc327a9f118fdbd17) --- lib/isc/netmgr/netmgr.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index 3283eb6e4f..0ed3182fb6 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -1746,8 +1746,12 @@ isc__nmhandle_detach(isc_nmhandle_t **handlep FLARG) { handle = *handlep; *handlep = NULL; + /* + * If the closehandle_cb is set, it needs to run asynchronously to + * ensure correct ordering of the isc__nm_process_sock_buffer(). + */ sock = handle->sock; - if (sock->tid == isc_nm_tid()) { + if (sock->tid == isc_nm_tid() && sock->closehandle_cb == NULL) { nmhandle_detach_cb(&handle FLARG_PASS); } else { isc__netievent_detach_t *event = -- 2.34.1