TLS: use isc_buffer_t for send requests

This commit replaces ad-hoc code for send requests buffer management
within TLS with the one based on isc_buffer_t.

Previous version of the code was trying to use pre-allocated small
buffers to avoid extra allocations. The code would allocate a larger
dynamic buffer when needed. There is no need to have ad-hoc code for
this, as isc_buffer_t now provides this functionality internally.

Additionally to the above, the old version of the code lacked any
logic to reuse the dynamically allocated buffers. Now, as we do not
manage memory buffers, but isc_buffer_t objects, we can implement this
strategy. It can be in particular helpful for longer lasting
connections, as in this case the buffer will adjust itself to the size
of the messages being transferred. That is, it is in particular useful
for XoT, as Stream DNS happen to order send requests in such a way
that the send request will get reused.

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index f71d74fb5aeb0869c027a47460ec0900732489f8..3a62bcce3a4d6683da4e49a9239c12365bbb0790 100644 (file)
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -735,7 +735,7 @@ typedef enum {
 
 typedef struct isc_nmsocket_tls_send_req {
        isc_nmsocket_t *tlssock;
-       isc_region_t data;
+       isc_buffer_t data;
        isc_nm_cb_t cb;
        void *cbarg;
        isc_nmhandle_t *handle;

diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c
index 8a337112552d2e87b569db6f54f6d3036eca5909..94c778577d135365eb59c0da242d964013bc529a 100644 (file)
--- a/lib/isc/netmgr/tlsstream.c
+++ b/lib/isc/netmgr/tlsstream.c
@@ -42,6 +42,8 @@
 
 #define TLS_BUF_SIZE (UINT16_MAX)
 
+#define TLS_MAX_SEND_BUF_SIZE (UINT16_MAX + UINT16_MAX / 2)
+
 static isc_result_t
 tls_error_to_result(const int tls_err, const int tls_state, isc_tls_t *tls) {
        switch (tls_err) {
@@ -142,7 +144,9 @@ tls_senddone(isc_nmhandle_t *handle, isc_result_t eresult, void *cbarg) {
        tlssock = send_req->tlssock;
        send_req->tlssock = NULL;
        send_cb = send_req->cb;
+       send_req->cb = NULL;
        send_cbarg = send_req->cbarg;
+       send_req->cbarg = NULL;
        send_handle = send_req->handle;
        send_req->handle = NULL;
 
@@ -150,27 +154,31 @@ tls_senddone(isc_nmhandle_t *handle, isc_result_t eresult, void *cbarg) {
                tls_try_shutdown(tlssock->tlsstream.tls, true);
        }
 
-       /*
-        * We are tying to avoid a memory allocation for small write
-        * requests. See the mirroring code in the tls_send_outgoing()
-        * function. The object is attempted to be freed or put for reuse
-        * before the call to callback because there is a chance that it
-        * is going to be reused during the call to the callback.
-        */
-       if (send_req->data.length > sizeof(send_req->smallbuf)) {
-               isc_mem_put(handle->sock->worker->mctx, send_req->data.base,
-                           send_req->data.length);
-       } else {
-               INSIST(&send_req->smallbuf[0] == send_req->data.base);
-       }
-
-       send_req->data.base = NULL;
-       send_req->data.length = 0;
-
        /* Try to keep the object to be reused later - to avoid an allocation */
        if (tlssock->tlsstream.send_req == NULL) {
                tlssock->tlsstream.send_req = send_req;
+               /*
+                * We need to ensure that the buffer is not going to grow too
+                * large uncontrollably. We try to keep its size to be no more
+                * than TLS_MAX_SEND_BUF_SIZE. The constant should be larger
+                * than 64 KB for this to work efficiently when combined with
+                * DNS transports.
+                */
+               if (isc_buffer_length(&send_req->data) > TLS_MAX_SEND_BUF_SIZE)
+               {
+                       /* free the underlying buffer */
+                       isc_buffer_clearmctx(&send_req->data);
+                       isc_buffer_invalidate(&send_req->data);
+                       isc_buffer_init(&send_req->data, send_req->smallbuf,
+                                       sizeof(send_req->smallbuf));
+                       isc_buffer_setmctx(&send_req->data,
+                                          handle->sock->worker->mctx);
+               } else {
+                       isc_buffer_clear(&send_req->data);
+               }
        } else {
+               isc_buffer_clearmctx(&send_req->data);
+               isc_buffer_invalidate(&send_req->data);
                isc_mem_put(handle->sock->worker->mctx, send_req,
                            sizeof(*send_req));
        }
@@ -274,6 +282,8 @@ tls_send_outgoing(isc_nmsocket_t *sock, bool finish, isc_nmhandle_t *tlshandle,
        int pending;
        int rv;
        size_t len = 0;
+       bool new_send_req = false;
+       isc_region_t used_region = { 0 };
 
        if (inactive(sock)) {
                if (cb != NULL) {
@@ -293,28 +303,23 @@ tls_send_outgoing(isc_nmsocket_t *sock, bool finish, isc_nmhandle_t *tlshandle,
                return (pending);
        }
 
-       /* TODO Should we keep track of these requests in a list? */
-       if ((unsigned int)pending > TLS_BUF_SIZE) {
-               pending = TLS_BUF_SIZE;
-       }
-
        /* Try to reuse previously allocated object */
        if (sock->tlsstream.send_req != NULL) {
                send_req = sock->tlsstream.send_req;
+               send_req->finish = finish;
                sock->tlsstream.send_req = NULL;
        } else {
                send_req = isc_mem_get(sock->worker->mctx, sizeof(*send_req));
+               *send_req = (isc_nmsocket_tls_send_req_t){ .finish = finish };
+               new_send_req = true;
        }
 
-       *send_req = (isc_nmsocket_tls_send_req_t){ .finish = finish,
-                                                  .data.length = pending };
-
-       /* Let's try to avoid a memory allocation for small write requests */
-       if ((size_t)pending > sizeof(send_req->smallbuf)) {
-               send_req->data.base = isc_mem_get(sock->worker->mctx, pending);
-       } else {
-               send_req->data.base = &send_req->smallbuf[0];
+       if (new_send_req) {
+               isc_buffer_init(&send_req->data, &send_req->smallbuf,
+                               sizeof(send_req->smallbuf));
+               isc_buffer_setmctx(&send_req->data, sock->worker->mctx);
        }
+       INSIST(isc_buffer_remaininglength(&send_req->data) == 0);
 
        isc__nmsocket_attach(sock, &send_req->tlssock);
        if (cb != NULL) {
@@ -323,15 +328,19 @@ tls_send_outgoing(isc_nmsocket_t *sock, bool finish, isc_nmhandle_t *tlshandle,
                isc_nmhandle_attach(tlshandle, &send_req->handle);
        }
 
-       rv = BIO_read_ex(sock->tlsstream.bio_out, send_req->data.base, pending,
-                        &len);
+       RUNTIME_CHECK(isc_buffer_reserve(&send_req->data, pending) ==
+                     ISC_R_SUCCESS);
+       isc_buffer_add(&send_req->data, pending);
+       rv = BIO_read_ex(sock->tlsstream.bio_out,
+                        isc_buffer_base(&send_req->data), pending, &len);
        /* There's something pending, read must succeed */
-       RUNTIME_CHECK(rv == 1);
+       RUNTIME_CHECK(rv == 1 && len == (size_t)pending);
 
        INSIST(VALID_NMHANDLE(sock->outerhandle));
 
        sock->tlsstream.nsending++;
-       isc_nm_send(sock->outerhandle, &send_req->data, tls_senddone, send_req);
+       isc_buffer_remainingregion(&send_req->data, &used_region);
+       isc_nm_send(sock->outerhandle, &used_region, tls_senddone, send_req);
 
        return (pending);
 }
@@ -1257,8 +1266,8 @@ isc__nm_tls_cleanup_data(isc_nmsocket_t *sock) {
                }
 
                if (sock->tlsstream.send_req != NULL) {
-                       INSIST(sock->tlsstream.send_req->data.base == NULL);
-                       INSIST(sock->tlsstream.send_req->data.length == 0);
+                       isc_buffer_clearmctx(&sock->tlsstream.send_req->data);
+                       isc_buffer_invalidate(&sock->tlsstream.send_req->data);
                        isc_mem_put(sock->worker->mctx,
                                    sock->tlsstream.send_req,
                                    sizeof(*sock->tlsstream.send_req));