From 098d2786f65769dc6a5de613acab86d6302eb997 Mon Sep 17 00:00:00 2001
From: Marek Vavrusa <marek@vavrusa.com>
Date: Mon, 11 Jul 2016 12:11:34 -0700
Subject: [PATCH] daemon/tls: fixed improper use of callback, leaks
the TLS sessions now bypass the usuall event loop asynchronous iops
this is because the whole operation is synchronous right now, and
implementing asynchronous send operations would require TLS session to
restart write events on the event loop and making sure the "on complete"
callback is called eventually
---
daemon/io.c | 2 +-
daemon/tls.c | 75 ++++++++-----------------------------------------
daemon/tls.h | 6 ++--
daemon/worker.c | 32 +++++++++++----------
4 files changed, 32 insertions(+), 83 deletions(-)
diff --git a/daemon/io.c b/daemon/io.c
index 1a69dbeeb..5d1e9837d 100644
--- a/daemon/io.c
+++ b/daemon/io.c
@@ -208,7 +208,7 @@ static void tcp_recv(uv_stream_t *handle, ssize_t nread, const uv_buf_t *buf)
* so the whole message reassembly and demuxing logic is inside worker */
int ret = 0;
if (s->has_tls) {
- ret = worker_process_tls(worker, handle, (const uint8_t *)buf->base, nread);
+ ret = tls_process(worker, handle, (const uint8_t *)buf->base, nread);
} else {
ret = worker_process_tcp(worker, handle, (const uint8_t *)buf->base, nread);
}
diff --git a/daemon/tls.c b/daemon/tls.c
index 56b449e8e..b6fdac836 100644
--- a/daemon/tls.c
+++ b/daemon/tls.c
@@ -41,10 +41,6 @@ struct tls_ctx_t {
ssize_t nread;
ssize_t consumed;
uint8_t recv_buf[4096];
-
- /* for writing to the network */
- uv_write_t *writer;
- uv_write_cb write_cb;
};
/** @internal Debugging facility. */
@@ -109,38 +105,6 @@ kres_gnutls_pull(gnutls_transport_ptr_t h, void *buf, size_t len)
return transfer;
}
-static ssize_t
-kres_gnutls_push_vec(gnutls_transport_ptr_t h, const giovec_t * iov, int iovcnt)
-{
- struct tls_ctx_t *t = (struct tls_ctx_t *)h;
- int ret;
-
- DEBUG_MSG("vecpush %d (%p) handle: <%p> writer <%p>\n", iovcnt, iov, t->handle, t->writer);
-
- /*
- * because of the struct of giovec_t is identical to struct iovec;
- * and uv_buf_t header (uv-unix.h) says it may be cast to struct
- * iovec; so we should be able to just cast directly.
- */
- ret = uv_write(t->writer, t->handle, (uv_buf_t *) iov, iovcnt, t->write_cb);
- if (ret >= 0) {
- /* Pending ioreq on current task */
- return (ssize_t) ret;
- }
- switch (ret) {
- case UV_EAGAIN:
- errno = EAGAIN;
- break;
- case UV_EINTR:
- errno = EINTR;
- break;
- default:
- kr_log_error("[tls] uv_write unknown error: %d\n", ret);
- errno = EIO; /* dkg just picked this at random */
- }
- return -1;
-}
-
struct tls_ctx_t *
tls_new(struct worker_ctx *worker)
{
@@ -236,65 +200,50 @@ tls_free(struct tls_ctx_t *tls)
free(tls);
}
-int
-push_tls(struct qr_task *task, uv_handle_t * handle, knot_pkt_t * pkt,
- uv_write_t * writer, qr_task_send_cb on_send)
+int tls_push(struct qr_task *task, uv_handle_t* handle, knot_pkt_t * pkt)
{
- ssize_t count;
- if (!pkt) {
- kr_log_error("[tls] cannot push null packet\n");
- return on_send(task, handle, kr_error(EIO));
+ if (!pkt || !handle || !handle->data) {
+ return kr_error(EINVAL);
}
- uint16_t pkt_size = htons(pkt->size);
+ ssize_t count;
struct session *session = handle->data;
- if (!session) {
- kr_log_error("[tls] no session on push\n");
- return on_send(task, handle, kr_error(EIO));
- }
+ uint16_t pkt_size = htons(pkt->size);
struct tls_ctx_t *tls_p = session->tls_ctx;
if (!tls_p) {
kr_log_error("[tls] no tls context on push\n");
- /* FIXME: might be necessary if we ever do outbound TLS */
- return on_send(task, handle, kr_error(EIO));
+ return kr_error(ENOENT);
}
- tls_p->handle = (uv_stream_t *) handle;
- tls_p->writer = writer;
gnutls_record_cork(tls_p->session);
count = gnutls_record_send(tls_p->session, &pkt_size, sizeof(pkt_size));
if (count != sizeof(pkt_size)) {
kr_log_error("[tls] gnutls_record_send pkt_size fail wanted: %u (%zd) %s\n",
pkt_size, count, gnutls_strerror_name(count));
- return on_send(task, handle, kr_error(EIO));
+ return kr_error(EIO);
}
count = gnutls_record_send(tls_p->session, pkt->wire, pkt->size);
if (count != pkt->size) {
kr_log_error("[tls] gnutls_record_send wire fail wanted: %zu (%zd) %s\n",
pkt->size, count, gnutls_strerror_name(count));
- return on_send(task, handle, kr_error(EIO));
+ return kr_error(EIO);
}
count = gnutls_record_uncork(tls_p->session, 0);
if (count != sizeof(pkt_size) + pkt->size) {
if (count == GNUTLS_E_AGAIN || count == GNUTLS_E_INTERRUPTED) {
kr_log_error("[tls] gnutls_record_send incomplete: %zu (%zd) %s\n",
pkt->size, count, gnutls_strerror_name(count));
- /*
- * FIXME: we need to know when this frees up; when it
- * does, we should do gnutls_record_send(tls.session,
- * NULL, 0); how do i know?
- */
+ return kr_error(EAGAIN);
} else {
kr_log_error("[tls] gnutls_record_send wire fail wanted: %zu (%zd) %s\n",
pkt->size, count, gnutls_strerror_name(count));
}
- return on_send(task, handle, kr_error(EIO));
+ return kr_error(EIO);
}
- return count;
+ return 0;
}
-int
-worker_process_tls(struct worker_ctx *worker, uv_stream_t * handle, const uint8_t * buf, ssize_t nread)
+int tls_process(struct worker_ctx *worker, uv_stream_t * handle, const uint8_t * buf, ssize_t nread)
{
struct session *session = handle->data;
struct tls_ctx_t *tls_p = session->tls_ctx;
diff --git a/daemon/tls.h b/daemon/tls.h
index 14646272b..fdce96bd7 100644
--- a/daemon/tls.h
+++ b/daemon/tls.h
@@ -24,7 +24,5 @@ struct tls_ctx_t;
struct tls_ctx_t* tls_new(struct worker_ctx *worker);
void tls_free(struct tls_ctx_t* tls);
-int push_tls(struct qr_task *task, uv_handle_t *handle, knot_pkt_t *pkt,
- uv_write_t *writer, qr_task_send_cb on_send);
-
-int worker_process_tls(struct worker_ctx *worker, uv_stream_t *handle, const uint8_t *buf, ssize_t nread);
+int tls_push(struct qr_task *task, uv_handle_t* handle, knot_pkt_t * pkt);
+int tls_process(struct worker_ctx *worker, uv_stream_t *handle, const uint8_t *buf, ssize_t nread);
diff --git a/daemon/worker.c b/daemon/worker.c
index 3a5380f11..1ab3d5d76 100644
--- a/daemon/worker.c
+++ b/daemon/worker.c
@@ -444,30 +444,32 @@ static int qr_task_send(struct qr_task *task, uv_handle_t *handle, struct sockad
if (!handle) {
return qr_task_on_send(task, handle, kr_error(EIO));
}
- struct req *send_req = req_borrow(task->worker);
- if (!send_req) {
- return qr_task_on_send(task, handle, kr_error(ENOMEM));
+
+ /* Synchronous push to TLS context, bypassing event loop. */
+ struct session *session = handle->data;
+ if (session->has_tls) {
+ int ret = tls_push(task, handle, pkt);
+ return qr_task_on_send(task, handle, ret);
}
/* Send using given protocol */
int ret = 0;
+ struct req *send_req = req_borrow(task->worker);
+ if (!send_req) {
+ return qr_task_on_send(task, handle, kr_error(ENOMEM));
+ }
if (handle->type == UV_UDP) {
uv_buf_t buf = { (char *)pkt->wire, pkt->size };
send_req->as.send.data = task;
ret = uv_udp_send(&send_req->as.send, (uv_udp_t *)handle, &buf, 1, addr, &on_send);
} else {
- struct session *session = handle->data;
- if (session->has_tls) {
- ret = push_tls(task, handle, pkt, &send_req->as.write, qr_task_on_send);
- } else {
- uint16_t pkt_size = htons(pkt->size);
- uv_buf_t buf[2] = {
- { (char *)&pkt_size, sizeof(pkt_size) },
- { (char *)pkt->wire, pkt->size }
- };
- send_req->as.write.data = task;
- ret = uv_write(&send_req->as.write, (uv_stream_t *)handle, buf, 2, &on_write);
- }
+ uint16_t pkt_size = htons(pkt->size);
+ uv_buf_t buf[2] = {
+ { (char *)&pkt_size, sizeof(pkt_size) },
+ { (char *)pkt->wire, pkt->size }
+ };
+ send_req->as.write.data = task;
+ ret = uv_write(&send_req->as.write, (uv_stream_t *)handle, buf, 2, &on_write);
}
if (ret == 0) {
qr_task_ref(task); /* Pending ioreq on current task */
--
GitLab