From 92a74b08af8805f3db74c47cbe233a1f67417069 Mon Sep 17 00:00:00 2001
From: Grigorii Demidov <grigorii.demidov@nic.cz>
Date: Thu, 30 Aug 2018 17:37:02 +0200
Subject: [PATCH 1/4] daemon/tls: avoid usage of gnutls_record_cork() \
gnutls_record_uncork(); use per-request io buffer
---
daemon/tls.c | 42 +++++++++++++++---------------------------
daemon/worker.c | 41 ++++++++++++++++++++++++++++++++++-------
2 files changed, 49 insertions(+), 34 deletions(-)
diff --git a/daemon/tls.c b/daemon/tls.c
index 5424cfc59..d442b894b 100644
--- a/daemon/tls.c
+++ b/daemon/tls.c
@@ -215,53 +215,41 @@ int tls_push(struct qr_task *task, uv_handle_t *handle, knot_pkt_t *pkt)
assert (tls_ctx);
assert (session->outgoing == tls_ctx->client_side);
- const uint16_t pkt_size = htons(pkt->size);
+ uint8_t *tcp_len_start = (uint8_t *)pkt->wire - 2;
const char *logstring = tls_ctx->client_side ? client_logstring : server_logstring;
gnutls_session_t tls_session = tls_ctx->tls_session;
+ const size_t size_to_send = pkt->size + 2;
+ ssize_t submitted = 0;
+ ssize_t retries = 0;
tls_ctx->task = task;
+ knot_wire_write_u16(tcp_len_start, pkt->size);
- assert(gnutls_record_check_corked(tls_session) == 0);
-
- gnutls_record_cork(tls_session);
- ssize_t count = 0;
- if ((count = gnutls_record_send(tls_session, &pkt_size, sizeof(pkt_size)) < 0) ||
- (count = gnutls_record_send(tls_session, pkt->wire, pkt->size) < 0)) {
- kr_log_error("[%s] gnutls_record_send failed: %s (%zd)\n",
- logstring, gnutls_strerror_name(count), count);
- return kr_error(EIO);
- }
-
- ssize_t submitted = 0;
- ssize_t retries = 0;
do {
- count = gnutls_record_uncork(tls_session, 0);
+ ssize_t count = gnutls_record_send(tls_session, &tcp_len_start[submitted], size_to_send);
if (count < 0) {
if (gnutls_error_is_fatal(count)) {
- kr_log_error("[%s] gnutls_record_uncork failed: %s (%zd)\n",
+ kr_log_error("[%s] gnutls_record_send failed: %s (%zd)\n",
logstring, gnutls_strerror_name(count), count);
+ assert(false);
return kr_error(EIO);
}
if (++retries > TLS_MAX_UNCORK_RETRIES) {
- kr_log_error("[%s] gnutls_record_uncork: too many sequential non-fatal errors (%zd), last error is: %s (%zd)\n",
+ kr_log_error("[%s] gnutls_record_send: too many sequential non-fatal errors (%zd), last error is: %s (%zd)\n",
logstring, retries, gnutls_strerror_name(count), count);
+ assert(false);
return kr_error(EIO);
}
} else if (count != 0) {
submitted += count;
retries = 0;
- } else if (gnutls_record_check_corked(tls_session) != 0) {
- if (++retries > TLS_MAX_UNCORK_RETRIES) {
- kr_log_error("[%s] gnutls_record_uncork: too many retries (%zd)\n",
- logstring, retries);
- return kr_error(EIO);
- }
- } else if (submitted != sizeof(pkt_size) + pkt->size) {
- kr_log_error("[%s] gnutls_record_uncork didn't send all data(%zd of %zd)\n",
- logstring, submitted, sizeof(pkt_size) + pkt->size);
+ } else if (submitted != size_to_send) {
+ kr_log_error("[%s] gnutls_record_send didn't send all data(%zd of %zd)\n",
+ logstring, submitted, size_to_send);
+ assert(false);
return kr_error(EIO);
}
- } while (submitted != sizeof(pkt_size) + pkt->size);
+ } while (submitted != size_to_send);
return kr_ok();
}
diff --git a/daemon/worker.c b/daemon/worker.c
index 850d02f56..bae140f88 100644
--- a/daemon/worker.c
+++ b/daemon/worker.c
@@ -62,6 +62,7 @@ struct qr_task
knot_pkt_t *pktbuf;
qr_tasklist_t waiting;
uv_handle_t *pending[MAX_PENDING];
+ void *write_req_buf;
uint16_t pending_count;
uint16_t addrlist_count;
uint16_t addrlist_turn;
@@ -586,7 +587,11 @@ static int request_start(struct request_ctx *ctx, knot_pkt_t *query)
}
req->qsource.size = query->size;
- req->answer = knot_pkt_new(NULL, answer_max, &req->pool);
+ /* Wire buffer + placeholder for tcp message length field.
+ * Placeholder allows to avoid usage of gnutls_record_cork() \ gnutls_record_cork()
+ * when TLS is used. */
+ char *wire = mm_alloc(&req->pool, answer_max + 2);
+ req->answer = knot_pkt_new(wire + 2, answer_max, &req->pool);
if (!req->answer) {
return kr_error(ENOMEM);
}
@@ -690,8 +695,12 @@ static struct qr_task *qr_task_create(struct request_ctx *ctx)
}
memset(task, 0, sizeof(*task)); /* avoid accidentally unitialized fields */
- /* Create packet buffers for answer and subrequests */
- knot_pkt_t *pktbuf = knot_pkt_new(NULL, pktbuf_max, &ctx->req.pool);
+ /* Create packet buffers for answer and subrequests.
+ * Don't forget about placeholder for tcp message length field.
+ * This allows to avoid usage of gnutls_record_cork() \ gnutls_record_cork()
+ * when TLS is used. */
+ char *wire = mm_alloc(&ctx->req.pool, pktbuf_max + 2);
+ knot_pkt_t *pktbuf = knot_pkt_new(wire + 2, pktbuf_max, &ctx->req.pool);
if (!pktbuf) {
mm_free(&ctx->req.pool, task);
return NULL;
@@ -895,6 +904,10 @@ static void on_task_write(uv_write_t *req, int status)
struct worker_ctx *worker = loop->data;
assert(worker == get_worker());
struct qr_task *task = req->data;
+ if (task->write_req_buf) {
+ free(task->write_req_buf);
+ task->write_req_buf = NULL;
+ }
qr_task_on_send(task, handle, status);
qr_task_unref(task);
iorequest_release(worker, req);
@@ -904,6 +917,10 @@ static void on_nontask_write(uv_write_t *req, int status)
{
uv_handle_t *handle = (uv_handle_t *)(req->handle);
uv_loop_t *loop = handle->loop;
+ if (req->data) {
+ free(req->data);
+ req->data = NULL;
+ }
struct worker_ctx *worker = loop->data;
assert(worker == get_worker());
iorequest_release(worker, req);
@@ -912,8 +929,12 @@ static void on_nontask_write(uv_write_t *req, int status)
ssize_t worker_gnutls_push(gnutls_transport_ptr_t h, const void *buf, size_t len)
{
struct tls_common_ctx *t = (struct tls_common_ctx *)h;
+
+ void *buf_local = malloc(len);
+ memcpy(buf_local, buf, len);
+
const uv_buf_t uv_buf[1] = {
- { (char *)buf, len }
+ { (char *)buf_local, len }
};
if (t == NULL) {
@@ -942,13 +963,14 @@ ssize_t worker_gnutls_push(gnutls_transport_ptr_t h, const void *buf, size_t len
uv_write_cb write_cb = on_task_write;
if (t->handshake_state == TLS_HS_DONE) {
assert(task);
+ write_req->data = task;
+ task->write_req_buf = buf_local;
} else {
task = NULL;
write_cb = on_nontask_write;
+ write_req->data = buf_local;
}
- write_req->data = task;
-
ssize_t ret = -1;
int res = uv_write(write_req, (uv_stream_t *)t->session->handle, uv_buf, 1, write_cb);
if (res == 0) {
@@ -2266,7 +2288,12 @@ int worker_process_tcp(struct worker_ctx *worker, uv_stream_t *handle,
* Previous packet is allocated with mempool, so there's no need to free it manually. */
if (task->pktbuf->max_size < KNOT_WIRE_MAX_PKTSIZE) {
knot_mm_t *pool = &task->pktbuf->mm;
- pkt_buf = knot_pkt_new(NULL, KNOT_WIRE_MAX_PKTSIZE, pool);
+ /* Allocate wire buffer + placeholder for tcp message length field.
+ * Placeholder allows to avoid usage of
+ * gnutls_record_cork() \ gnutls_record_cork()
+ * when TLS is used. */
+ char *wire = mm_alloc(pool, KNOT_WIRE_MAX_PKTSIZE + 2);
+ pkt_buf = knot_pkt_new(wire + 2, KNOT_WIRE_MAX_PKTSIZE, pool);
if (!pkt_buf) {
return kr_error(ENOMEM);
}
--
GitLab
From 34ab5025e41f2655e1bb29000a7ad18ceda73778 Mon Sep 17 00:00:00 2001
From: Grigorii Demidov <grigorii.demidov@nic.cz>
Date: Fri, 31 Aug 2018 15:26:49 +0200
Subject: [PATCH 2/4] daemon/tls: minor refactoring of tls_push()
---
daemon/tls.c | 42 ++++++++++++++----------------------------
1 file changed, 14 insertions(+), 28 deletions(-)
diff --git a/daemon/tls.c b/daemon/tls.c
index d442b894b..e034b0594 100644
--- a/daemon/tls.c
+++ b/daemon/tls.c
@@ -208,6 +208,7 @@ int tls_push(struct qr_task *task, uv_handle_t *handle, knot_pkt_t *pkt)
return kr_error(EINVAL);
}
+ int ret = kr_ok();
struct session *session = handle->data;
struct tls_common_ctx *tls_ctx = session->outgoing ? &session->tls_client_ctx->c :
&session->tls_ctx->c;
@@ -219,39 +220,24 @@ int tls_push(struct qr_task *task, uv_handle_t *handle, knot_pkt_t *pkt)
const char *logstring = tls_ctx->client_side ? client_logstring : server_logstring;
gnutls_session_t tls_session = tls_ctx->tls_session;
const size_t size_to_send = pkt->size + 2;
- ssize_t submitted = 0;
- ssize_t retries = 0;
tls_ctx->task = task;
knot_wire_write_u16(tcp_len_start, pkt->size);
- do {
- ssize_t count = gnutls_record_send(tls_session, &tcp_len_start[submitted], size_to_send);
- if (count < 0) {
- if (gnutls_error_is_fatal(count)) {
- kr_log_error("[%s] gnutls_record_send failed: %s (%zd)\n",
- logstring, gnutls_strerror_name(count), count);
- assert(false);
- return kr_error(EIO);
- }
- if (++retries > TLS_MAX_UNCORK_RETRIES) {
- kr_log_error("[%s] gnutls_record_send: too many sequential non-fatal errors (%zd), last error is: %s (%zd)\n",
- logstring, retries, gnutls_strerror_name(count), count);
- assert(false);
- return kr_error(EIO);
- }
- } else if (count != 0) {
- submitted += count;
- retries = 0;
- } else if (submitted != size_to_send) {
- kr_log_error("[%s] gnutls_record_send didn't send all data(%zd of %zd)\n",
- logstring, submitted, size_to_send);
- assert(false);
- return kr_error(EIO);
- }
- } while (submitted != size_to_send);
+ /* gnutls_record_send() calls worker_gnutls_push() which actually sends data.
+ * It either sends all the data either sends nothing and returns error. */
+ ssize_t count = gnutls_record_send(tls_session, tcp_len_start, size_to_send);
+ if (count < 0) {
+ kr_log_error("[%s] gnutls_record_send failed: %s (%zd)\n",
+ logstring, gnutls_strerror_name(count), count);
+ ret = kr_error(EIO);
+ } else if (count != size_to_send) {
+ kr_log_error("[%s] gnutls_record_send didn't send all data (%zd of %zd)\n",
+ logstring, count, size_to_send);
+ ret = kr_error(EIO);
+ }
- return kr_ok();
+ return ret;
}
int tls_process(struct worker_ctx *worker, uv_stream_t *handle, const uint8_t *buf, ssize_t nread)
--
GitLab
From 1d4f501cfc5b9715b9b5af177868fe33b413c971 Mon Sep 17 00:00:00 2001
From: Grigorii Demidov <grigorii.demidov@nic.cz>
Date: Fri, 31 Aug 2018 16:02:44 +0200
Subject: [PATCH 3/4] daemon/worker: in some circumstances task can be
finalized twice, fixed
---
daemon/worker.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/daemon/worker.c b/daemon/worker.c
index bae140f88..7cd0e983c 100644
--- a/daemon/worker.c
+++ b/daemon/worker.c
@@ -853,25 +853,26 @@ static int qr_task_on_send(struct qr_task *task, uv_handle_t *handle, int status
if (ret != kr_ok()) {
while (session->waiting.len > 0) {
struct qr_task *t = session->waiting.at[0];
+ array_del(session->waiting, 0);
+ session_del_tasks(session, t);
if (session->outgoing) {
qr_task_finalize(t, KR_STATE_FAIL);
} else {
assert(t->ctx->source.session == session);
t->ctx->source.session = NULL;
}
- array_del(session->waiting, 0);
- session_del_tasks(session, t);
qr_task_unref(t);
}
while (session->tasks.len > 0) {
struct qr_task *t = session->tasks.at[0];
+ array_del(session->tasks, 0);
if (session->outgoing) {
qr_task_finalize(t, KR_STATE_FAIL);
} else {
assert(t->ctx->source.session == session);
t->ctx->source.session = NULL;
}
- session_del_tasks(session, t);
+ qr_task_unref(t);
}
session_close(session);
return status;
@@ -1767,8 +1768,9 @@ static int qr_task_step(struct qr_task *task,
session_del_tasks(session, task);
while (session->tasks.len != 0) {
struct qr_task *t = session->tasks.at[0];
+ array_del(session->tasks, 0);
qr_task_finalize(t, KR_STATE_FAIL);
- session_del_tasks(session, t);
+ qr_task_unref(t);
}
subreq_finalize(task, packet_source, packet);
session_close(session);
@@ -1784,8 +1786,9 @@ static int qr_task_step(struct qr_task *task,
session_del_tasks(session, task);
while (session->tasks.len != 0) {
struct qr_task *t = session->tasks.at[0];
+ array_del(session->tasks, 0);
qr_task_finalize(t, KR_STATE_FAIL);
- session_del_tasks(session, t);
+ qr_task_unref(t);
}
subreq_finalize(task, packet_source, packet);
session_close(session);
--
GitLab
From 6a1b4b92cf025e2ff9ef361356fde8fa424a491a Mon Sep 17 00:00:00 2001
From: Grigorii Demidov <grigorii.demidov@nic.cz>
Date: Mon, 3 Sep 2018 17:00:04 +0200
Subject: [PATCH 4/4] daemon: fix memory leaks
---
daemon/worker.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/daemon/worker.c b/daemon/worker.c
index 7cd0e983c..219e19acc 100644
--- a/daemon/worker.c
+++ b/daemon/worker.c
@@ -728,6 +728,10 @@ static void qr_task_free(struct qr_task *task)
assert(ctx);
+ if (task->write_req_buf != NULL) {
+ free(task->write_req_buf);
+ }
+
/* Process outbound session. */
struct session *source_session = ctx->source.session;
struct worker_ctx *worker = ctx->worker;
@@ -930,6 +934,10 @@ static void on_nontask_write(uv_write_t *req, int status)
ssize_t worker_gnutls_push(gnutls_transport_ptr_t h, const void *buf, size_t len)
{
struct tls_common_ctx *t = (struct tls_common_ctx *)h;
+ if (t == NULL) {
+ errno = EFAULT;
+ return -1;
+ }
void *buf_local = malloc(len);
memcpy(buf_local, buf, len);
@@ -938,11 +946,6 @@ ssize_t worker_gnutls_push(gnutls_transport_ptr_t h, const void *buf, size_t len
{ (char *)buf_local, len }
};
- if (t == NULL) {
- errno = EFAULT;
- return -1;
- }
-
assert(t->session && t->session->handle &&
t->session->handle->type == UV_TCP);
@@ -955,6 +958,7 @@ ssize_t worker_gnutls_push(gnutls_transport_ptr_t h, const void *buf, size_t len
void *ioreq = worker_iohandle_borrow(worker);
if (!ioreq) {
errno = EFAULT;
+ free(buf_local);
return -1;
}
@@ -964,6 +968,9 @@ ssize_t worker_gnutls_push(gnutls_transport_ptr_t h, const void *buf, size_t len
uv_write_cb write_cb = on_task_write;
if (t->handshake_state == TLS_HS_DONE) {
assert(task);
+ if (task->write_req_buf != NULL) {
+ free(task->write_req_buf);
+ }
write_req->data = task;
task->write_req_buf = buf_local;
} else {
@@ -998,6 +1005,7 @@ ssize_t worker_gnutls_push(gnutls_transport_ptr_t h, const void *buf, size_t len
VERBOSE_MSG(NULL,"[%s] uv_write: %s\n",
t->client_side ? "tls_client" : "tls", uv_strerror(res));
iorequest_release(worker, ioreq);
+ free(buf_local);
errno = EIO;
}
return ret;
--
GitLab