lib: fix c-ares thread misuse

The `struct thread **ref` that the thread code takes is written to and
needs to stay valid over the lifetime of a thread.  This does not hold
up if thread pointers are directly put in a `vector` since adding items
to a `vector` may reallocate the entire array.  The thread code would
then write to a now-invalid `ref`, potentially corrupting entirely
unrelated data.

This should be extremely rare to trigger in practice since we only use
one c-ares channel, which will likely only ever use one fd, so the
vector is never resized.  That said, c-ares using only one fd is just
plain fragile luck.

Either way, fix this by creating a resolver_fd tracking struct, and
clean up the code while we're at it.

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>

1 files changed, 110 insertions, 68 deletions

diff --git a/lib/resolver.c b/lib/resolver.c
index 4aba909f2..e3dba5f8a 100644
--- a/lib/resolver.c
+++ b/lib/resolver.c
@@ -14,7 +14,8 @@
 #include <ares.h>
 #include <ares_version.h>
 
-#include "vector.h"
+#include "typesafe.h"
+#include "jhash.h"
 #include "thread.h"
 #include "lib_errors.h"
 #include "resolver.h"
@@ -27,13 +28,78 @@ struct resolver_state {
 	ares_channel channel;
 	struct thread_master *master;
 	struct thread *timeout;
-	vector read_threads, write_threads;
 };
 
 static struct resolver_state state;
 static bool resolver_debug;
 
-#define THREAD_RUNNING ((struct thread *)-1)
+/* a FD doesn't necessarily map 1:1 to a request;  we could be talking to
+ * multiple caches simultaneously, to see which responds fastest.
+ * Theoretically we could also be using the same fd for multiple lookups,
+ * but the c-ares API guarantees an n:1 mapping for fd => channel.
+ *
+ * Either way c-ares makes that decision and we just need to deal with
+ * whatever FDs it gives us.
+ */
+
+DEFINE_MTYPE_STATIC(LIB, ARES_FD, "c-ares (DNS) file descriptor information");
+PREDECL_HASH(resolver_fds);
+
+struct resolver_fd {
+	struct resolver_fds_item itm;
+
+	int fd;
+	struct resolver_state *state;
+	struct thread *t_read, *t_write;
+};
+
+static int resolver_fd_cmp(const struct resolver_fd *a,
+			   const struct resolver_fd *b)
+{
+	return numcmp(a->fd, b->fd);
+}
+
+static uint32_t resolver_fd_hash(const struct resolver_fd *item)
+{
+	return jhash_1word(item->fd, 0xacd04c9e);
+}
+
+DECLARE_HASH(resolver_fds, struct resolver_fd, itm, resolver_fd_cmp,
+	     resolver_fd_hash);
+
+static struct resolver_fds_head resfds[1] = {INIT_HASH(resfds[0])};
+
+static struct resolver_fd *resolver_fd_get(int fd,
+					   struct resolver_state *newstate)
+{
+	struct resolver_fd ref = {.fd = fd}, *res;
+
+	res = resolver_fds_find(resfds, &ref);
+	if (!res && newstate) {
+		res = XCALLOC(MTYPE_ARES_FD, sizeof(*res));
+		res->fd = fd;
+		res->state = newstate;
+		resolver_fds_add(resfds, res);
+
+		if (resolver_debug)
+			zlog_debug("c-ares registered FD %d", fd);
+	}
+	return res;
+}
+
+static void resolver_fd_drop_maybe(struct resolver_fd *resfd)
+{
+	if (resfd->t_read || resfd->t_write)
+		return;
+
+	if (resolver_debug)
+		zlog_debug("c-ares unregistered FD %d", resfd->fd);
+
+	resolver_fds_del(resfds, resfd);
+	XFREE(MTYPE_ARES_FD, resfd);
+}
+
+/* end of FD housekeeping */
 
 static void resolver_update_timeouts(struct resolver_state *r);
 
@@ -41,9 +107,7 @@ static int resolver_cb_timeout(struct thread *t)
 {
 	struct resolver_state *r = THREAD_ARG(t);
 
-	r->timeout = THREAD_RUNNING;
 	ares_process(r->channel, NULL, NULL);
-	r->timeout = NULL;
 	resolver_update_timeouts(r);
 
 	return 0;
@@ -51,17 +115,16 @@ static int resolver_cb_timeout(struct thread *t)
 
 static int resolver_cb_socket_readable(struct thread *t)
 {
-	struct resolver_state *r = THREAD_ARG(t);
-	int fd = THREAD_FD(t);
-	struct thread **t_ptr;
-
-	vector_set_index(r->read_threads, fd, THREAD_RUNNING);
-	ares_process_fd(r->channel, fd, ARES_SOCKET_BAD);
-	if (vector_lookup(r->read_threads, fd) == THREAD_RUNNING) {
-		t_ptr = (struct thread **)vector_get_index(r->read_threads, fd);
-		thread_add_read(r->master, resolver_cb_socket_readable, r, fd,
-				t_ptr);
-	}
+	struct resolver_fd *resfd = THREAD_ARG(t);
+	struct resolver_state *r = resfd->state;
+
+	thread_add_read(r->master, resolver_cb_socket_readable, resfd,
+			resfd->fd, &resfd->t_read);
+	/* ^ ordering important:
+	 * ares_process_fd may transitively call THREAD_OFF(resfd->t_read)
+	 * combined with resolver_fd_drop_maybe, so resfd may be free'd after!
+	 */
+	ares_process_fd(r->channel, resfd->fd, ARES_SOCKET_BAD);
 	resolver_update_timeouts(r);
 
 	return 0;
@@ -69,17 +132,16 @@ static int resolver_cb_socket_readable(struct thread *t)
 
 static int resolver_cb_socket_writable(struct thread *t)
 {
-	struct resolver_state *r = THREAD_ARG(t);
-	int fd = THREAD_FD(t);
-	struct thread **t_ptr;
-
-	vector_set_index(r->write_threads, fd, THREAD_RUNNING);
-	ares_process_fd(r->channel, ARES_SOCKET_BAD, fd);
-	if (vector_lookup(r->write_threads, fd) == THREAD_RUNNING) {
-		t_ptr = (struct thread **)vector_get_index(r->write_threads, fd);
-		thread_add_write(r->master, resolver_cb_socket_writable, r, fd,
-				 t_ptr);
-	}
+	struct resolver_fd *resfd = THREAD_ARG(t);
+	struct resolver_state *r = resfd->state;
+
+	thread_add_write(r->master, resolver_cb_socket_writable, resfd,
+			 resfd->fd, &resfd->t_write);
+	/* ^ ordering important:
+	 * ares_process_fd may transitively call THREAD_OFF(resfd->t_write)
+	 * combined with resolver_fd_drop_maybe, so resfd may be free'd after!
+	 */
+	ares_process_fd(r->channel, ARES_SOCKET_BAD, resfd->fd);
 	resolver_update_timeouts(r);
 
 	return 0;
@@ -89,13 +151,11 @@ static void resolver_update_timeouts(struct resolver_state *r)
 {
 	struct timeval *tv, tvbuf;
 
-	if (r->timeout == THREAD_RUNNING)
-		return;
-
 	THREAD_OFF(r->timeout);
 	tv = ares_timeout(r->channel, NULL, &tvbuf);
 	if (tv) {
 		unsigned int timeoutms = tv->tv_sec * 1000 + tv->tv_usec / 1000;
+
 		thread_add_timer_msec(r->master, resolver_cb_timeout, r,
 				      timeoutms, &r->timeout);
 	}
@@ -105,43 +165,27 @@ static void ares_socket_cb(void *data, ares_socket_t fd, int readable,
 			   int writable)
 {
 	struct resolver_state *r = (struct resolver_state *)data;
-	struct thread *t, **t_ptr;
-
-	if (readable) {
-		t = vector_lookup(r->read_threads, fd);
-		if (!t) {
-			t_ptr = (struct thread **)vector_get_index(
-				r->read_threads, fd);
-			thread_add_read(r->master, resolver_cb_socket_readable,
-					r, fd, t_ptr);
-		}
-	} else {
-		t = vector_lookup(r->read_threads, fd);
-		if (t) {
-			if (t != THREAD_RUNNING) {
-				THREAD_OFF(t);
-			}
-			vector_unset(r->read_threads, fd);
-		}
-	}
+	struct resolver_fd *resfd;
 
-	if (writable) {
-		t = vector_lookup(r->write_threads, fd);
-		if (!t) {
-			t_ptr = (struct thread **)vector_get_index(
-				r->write_threads, fd);
-			thread_add_read(r->master, resolver_cb_socket_writable,
-					r, fd, t_ptr);
-		}
-	} else {
-		t = vector_lookup(r->write_threads, fd);
-		if (t) {
-			if (t != THREAD_RUNNING) {
-				THREAD_OFF(t);
-			}
-			vector_unset(r->write_threads, fd);
-		}
-	}
+	resfd = resolver_fd_get(fd, (readable || writable) ? r : NULL);
+	if (!resfd)
+		return;
+
+	assert(resfd->state == r);
+
+	if (!readable)
+		THREAD_OFF(resfd->t_read);
+	else if (!resfd->t_read)
+		thread_add_read(r->master, resolver_cb_socket_readable, resfd,
+				fd, &resfd->t_read);
+
+	if (!writable)
+		THREAD_OFF(resfd->t_write);
+	else if (!resfd->t_write)
+		thread_add_write(r->master, resolver_cb_socket_writable, resfd,
+				 fd, &resfd->t_write);
+
+	resolver_fd_drop_maybe(resfd);
 }
 
 
@@ -271,8 +315,6 @@ void resolver_init(struct thread_master *tm)
 	struct ares_options ares_opts;
 
 	state.master = tm;
-	state.read_threads = vector_init(1);
-	state.write_threads = vector_init(1);
 
 	ares_opts = (struct ares_options){
 		.sock_state_cb = &ares_socket_cb,