From 8b19f62ac59ed51fc10add3f9239abe78626babc Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 13 Apr 2022 16:24:20 +0300 Subject: [PATCH] TLSDNS: call send callbacks after only the data was sent This commit ensures that write callbacks are getting called only after the data has been sent via the network. Without this fix, a situation could appear when a write callback could get called before the actual encrypted data would have been sent to the network. Instead, it would get called right after it would have been passed to the OpenSSL (i.e. encrypted). Most likely, the issue does not reveal itself often because the callback call was asynchronous, so in most cases it should have been called after the data has been sent, but that was not guaranteed by the code logic. Also, this commit removes one memory allocation (netievent) from a hot path, as there is no need to call this callback asynchronously anymore. --- lib/isc/netmgr/netmgr-int.h | 1 + lib/isc/netmgr/netmgr.c | 4 ++++ lib/isc/netmgr/tlsdns.c | 35 +++++++++++++++++++++++++++-------- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 8d8684602b..d9f8f5a3c3 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -916,6 +916,7 @@ struct isc_nmsocket { TLS_STATE_CLOSING } state; isc_region_t senddata; + ISC_LIST(isc__nm_uvreq_t) sendreqs; bool cycle; isc_result_t pending_error; /* List of active send requests. */ diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index 4c0b96b123..313c11402d 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -1280,6 +1280,8 @@ nmsocket_cleanup(isc_nmsocket_t *sock, bool dofree FLARG) { isc__nm_tls_cleanup_data(sock); isc__nm_http_cleanup_data(sock); #endif + + INSIST(ISC_LIST_EMPTY(sock->tls.sendreqs)); #ifdef NETMGR_TRACE LOCK(&sock->mgr->lock); ISC_LIST_UNLINK(sock->mgr->active_sockets, sock, active_link); @@ -1468,6 +1470,8 @@ isc___nmsocket_init(isc_nmsocket_t *sock, isc_nm_t *mgr, isc_nmsocket_type type, .inactivereqs = isc_astack_new( mgr->mctx, ISC_NM_REQS_STACK_SIZE) }; + ISC_LIST_INIT(sock->tls.sendreqs); + if (iface != NULL) { family = iface->type.sa.sa_family; sock->iface = *iface; diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c index a20398789e..216a0e12c9 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -77,6 +77,9 @@ async_tlsdns_cycle(isc_nmsocket_t *sock) __attribute__((unused)); static isc_result_t tls_cycle(isc_nmsocket_t *sock); +static void +call_pending_send_callbacks(isc_nmsocket_t *sock, const isc_result_t result); + static bool can_log_tlsdns_quota(void) { isc_stdtime_t now, last; @@ -820,6 +823,7 @@ isc__nm_tlsdns_failed_read_cb(isc_nmsocket_t *sock, isc_result_t result, } destroy: + call_pending_send_callbacks(sock, result); isc__nmsocket_prep_destroy(sock); /* @@ -1149,7 +1153,19 @@ tls_error(isc_nmsocket_t *sock, isc_result_t result) { } static void -free_senddata(isc_nmsocket_t *sock) { +call_pending_send_callbacks(isc_nmsocket_t *sock, const isc_result_t result) { + isc__nm_uvreq_t *cbreq = ISC_LIST_HEAD(sock->tls.sendreqs); + while (cbreq != NULL) { + isc__nm_uvreq_t *next = ISC_LIST_NEXT(cbreq, link); + ISC_LIST_UNLINK(sock->tls.sendreqs, cbreq, link); + INSIST(sock == cbreq->handle->sock); + isc__nm_sendcb(sock, cbreq, result, false); + cbreq = next; + } +} + +static void +free_senddata(isc_nmsocket_t *sock, const isc_result_t result) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->tls.senddata.base != NULL); REQUIRE(sock->tls.senddata.length > 0); @@ -1158,23 +1174,26 @@ free_senddata(isc_nmsocket_t *sock) { sock->tls.senddata.length); sock->tls.senddata.base = NULL; sock->tls.senddata.length = 0; + + call_pending_send_callbacks(sock, result); } static void tls_write_cb(uv_write_t *req, int status) { - isc_result_t result; + isc_result_t result = status != 0 ? isc__nm_uverr2result(status) + : ISC_R_SUCCESS; isc__nm_uvreq_t *uvreq = (isc__nm_uvreq_t *)req->data; isc_nmsocket_t *sock = uvreq->sock; isc_nm_timer_stop(uvreq->timer); isc_nm_timer_detach(&uvreq->timer); - free_senddata(sock); + free_senddata(sock, result); isc__nm_uvreq_put(&uvreq, sock); if (status != 0) { - tls_error(sock, isc__nm_uverr2result(status)); + tls_error(sock, result); return; } @@ -1224,7 +1243,7 @@ tls_cycle_output(isc_nmsocket_t *sock) { if (r == pending) { /* Wrote everything, restart */ isc__nm_uvreq_put(&req, sock); - free_senddata(sock); + free_senddata(sock, ISC_R_SUCCESS); continue; } @@ -1239,7 +1258,7 @@ tls_cycle_output(isc_nmsocket_t *sock) { } else { result = isc__nm_uverr2result(r); isc__nm_uvreq_put(&req, sock); - free_senddata(sock); + free_senddata(sock, result); break; } @@ -1248,7 +1267,7 @@ tls_cycle_output(isc_nmsocket_t *sock) { if (r < 0) { result = isc__nm_uverr2result(r); isc__nm_uvreq_put(&req, sock); - free_senddata(sock); + free_senddata(sock, result); break; } @@ -1727,7 +1746,7 @@ tlsdns_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) { /* SSL_write_ex() doesn't do partial writes */ INSIST(sendlen == bytes); - isc__nm_sendcb(sock, req, ISC_R_SUCCESS, true); + ISC_LIST_APPEND(sock->tls.sendreqs, req, link); async_tlsdns_cycle(sock); return (ISC_R_SUCCESS); }