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.

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index 9adb7e0084db35f36167ead365180b7b475162b3..eb4b14e368f1ab3d8cd384288dd95022714512cc 100644 (file)
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -948,6 +948,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 df8065daf5e7549b69d51037073b392a146711c9..f7444630c869100e6fe5b900092ea482fc3f2ee5 100644 (file)
--- a/lib/isc/netmgr/netmgr.c
+++ b/lib/isc/netmgr/netmgr.c
@@ -1435,6 +1435,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 afbb3db2acf0b676d1d657112c86fadcfc3740cb..68fbca67fe242520775f304417089f9cc533138f 100644 (file)
--- 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
 peer_verification_has_failed(isc_nmsocket_t *sock) {
        if (sock->tls.tls != NULL && sock->tls.state == TLS_STATE_HANDSHAKE &&
@@ -835,6 +838,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);
 
        /*
@@ -1164,7 +1168,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);
@@ -1173,23 +1189,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;
        }
 
@@ -1239,7 +1258,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;
                }
 
@@ -1254,7 +1273,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;
                }
 
@@ -1263,7 +1282,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;
                }
 
@@ -1741,7 +1760,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);
        }
@@ -2132,6 +2151,7 @@ isc__nm_tlsdns_cleanup_data(isc_nmsocket_t *sock) {
             sock->type == isc_nm_tlsdnssocket) &&
            sock->tls.ctx != NULL)
        {
+               INSIST(ISC_LIST_EMPTY(sock->tls.sendreqs));
                isc_tlsctx_free(&sock->tls.ctx);
        }
 }