Commit Diff

Commit:
207b3e80d867693ff74cf99c84f7dd41386adba1
From:
Omar Polo <op@omarpolo.com>
Date:
Message:
Store clients inside a splay tree From day one we've been using a static array of client struct to hold the clients data. This has variuos drawbacks, among which: * reuse of the storage ("shades of heartbleed") * maximum fixed amount of clients connected at the same time * bugs are harder to debug The last point in particular is important because if we mess the client ids, or try to execute some functions (e.g. the various fcgi_*) after a client has been disconnected, it's harder to "see" this "use after free"-tier kind of bug. Now I'm using a splay tree to hold the data about the live connections. Each client' data is managed by malloc. If we try to access a client data after the disconnection we'll probably crash with a SIGSEGV and find the bug is more easy. Performance-wise the connection phase should be faster since we don't have to loop anymore to find an empty spot in the clients array, but some operations could be slightly slower (compare the O(1) access in an array with a SPLAY_FIND operation -- still be faster than O(n) thought.)
Actions:
Patch | Tree
commit - 4cd25209651f224be8c34d6006ef689963ce37d5
commit + 207b3e80d867693ff74cf99c84f7dd41386adba1
blob - 43c684fec0d931c7f6845d1a37fb8ee624a2db59
blob + 4a8befc7db5c209d20eea8e4a812b0cb8b5ee6dc
--- gmid.h
+++ gmid.h
@@ -20,6 +20,7 @@
 #include "config.h"
 
 #include <sys/socket.h>
+#include <sys/tree.h>
 #include <sys/types.h>
 
 #include <arpa/inet.h>
@@ -65,8 +66,6 @@
 #define CLIENT_CERT_REQ	60
 #define CERT_NOT_AUTH	61
 
-#define MAX_USERS	64
-
 /* maximum hostname and label length, +1 for the NUL-terminator */
 #define DOMAIN_NAME_LEN	(253+1)
 #define LABEL_LEN	(63+1)
@@ -200,7 +199,7 @@ enum {
 #define IS_INTERNAL_REQUEST(x)	((x) != REQUEST_CGI && (x) != REQUEST_FCGI)
 
 struct client {
-	int		 id;
+	uint32_t	 id;
 	struct tls	*ctx;
 	char		*req;
 	struct iri	 iri;
@@ -227,9 +226,11 @@ struct client {
 	struct sockaddr_storage	 addr;
 	struct vhost	*host;	/* host they're talking to */
 	size_t		 loc;	/* location matched */
-};
 
-extern struct client clients[MAX_USERS];
+	SPLAY_ENTRY(client) entry;
+};
+SPLAY_HEAD(client_tree_id, client);
+extern struct client_tree_id clients;
 
 struct cgireq {
 	char		buf[GEMINI_URL_LEN];
@@ -333,6 +334,9 @@ void		 client_close(struct client *);
 struct client	*try_client_by_id(int);
 void		 loop(struct tls*, int, int, struct imsgbuf*);
 
+int		 client_tree_cmp(struct client *, struct client *);
+SPLAY_PROTOTYPE(client_tree_id, client, entry, client_tree_cmp);
+
 /* dirs.c */
 int		 scandir_fd(int, struct dirent***, int(*)(const struct dirent*),
 		    int(*)(const struct dirent**, const struct dirent**));
blob - 66202c460bbf1104741d192306a0c680e81a48c2
blob + 4f5e4f2d0a2c7a443b18b8faf51543ed71d090e6
--- server.c
+++ server.c
@@ -31,8 +31,6 @@
 
 int shutting_down;
 
-struct client	 clients[MAX_USERS];
-
 static struct tls	*ctx;
 
 static struct event e4, e6, imsgev, siginfo, sigusr2;
@@ -83,6 +81,10 @@ static imsg_handlerfn *handlers[] = {
 	[IMSG_CGI_RES] = handle_imsg_cgi_res,
 	[IMSG_FCGI_FD] = handle_imsg_fcgi_fd,
 };
+
+static uint32_t server_client_id;
+
+struct client_tree_id clients;
 
 static inline int
 matches(const char *pattern, const char *path)
@@ -1164,6 +1166,8 @@ client_close(struct client *c)
 	 * ensure that everything is properly released once we reach
 	 * this point.
 	 */
+
+	SPLAY_REMOVE(client_tree_id, &clients, c);
 
 	if (c->cgibev != NULL) {
 		bufferevent_disable(c->cgibev, EVBUFFER_READ|EVBUFFER_WRITE);
@@ -1275,7 +1279,7 @@ do_accept(int sock, short et, void *d)
 	struct sockaddr_storage addr;
 	struct sockaddr *saddr;
 	socklen_t len;
-	int i, fd;
+	int fd;
 
 	(void)et;
 
@@ -1289,44 +1293,41 @@ do_accept(int sock, short et, void *d)
 
 	mark_nonblock(fd);
 
-	for (i = 0; i < MAX_USERS; ++i) {
-		c = &clients[i];
-		if (c->fd == -1) {
-			memset(c, 0, sizeof(*c));
-			c->id = i;
-			if (tls_accept_socket(ctx, &c->ctx, fd) == -1)
-				break; /* goodbye fd! */
+	c = xcalloc(1, sizeof(*c));
+	c->id = ++server_client_id;
+	c->fd = fd;
+	c->pfd = -1;
+	c->addr = addr;
 
-			c->fd = fd;
-			c->pfd = -1;
-			c->dir = NULL;
-			c->addr = addr;
-
-			event_once(c->fd, EV_READ|EV_WRITE, handle_handshake,
-			    c, NULL);
-
-			connected_clients++;
-			return;
-		}
+	if (tls_accept_socket(ctx, &c->ctx, fd) == -1) {
+		log_warn(c, "failed to accept socket: %s", tls_error(c->ctx));
+		close(c->fd);
+		free(c);
+		return;
 	}
 
-	close(fd);
+	SPLAY_INSERT(client_tree_id, &clients, c);
+	event_once(c->fd, EV_READ|EV_WRITE, handle_handshake, c, NULL);
+	connected_clients++;
 }
 
 static struct client *
 client_by_id(int id)
 {
-	if ((size_t)id > sizeof(clients)/sizeof(clients[0]))
+	struct client *c;
+
+	if ((c = try_client_by_id(id)) == NULL)
 		fatal("in client_by_id: invalid id %d", id);
-	return &clients[id];
+	return c;
 }
 
 struct client *
 try_client_by_id(int id)
 {
-	if ((size_t)id > sizeof(clients)/sizeof(clients[0]))
-                return NULL;
-	return &clients[id];
+	struct client find;
+
+	find.id = id;
+	return SPLAY_FIND(client_tree_id, &clients, &find);
 }
 
 static void
@@ -1423,16 +1424,12 @@ handle_siginfo(int fd, short ev, void *d)
 void
 loop(struct tls *ctx_, int sock4, int sock6, struct imsgbuf *ibuf)
 {
-	size_t i;
-
 	ctx = ctx_;
 
+	SPLAY_INIT(&clients);
+
 	event_init();
 
-	memset(&clients, 0, sizeof(clients));
-	for (i = 0; i < MAX_USERS; ++i)
-		clients[i].fd = -1;
-
 	event_set(&e4, sock4, EV_READ | EV_PERSIST, &do_accept, NULL);
 	event_add(&e4, NULL);
 
@@ -1457,3 +1454,16 @@ loop(struct tls *ctx_, int sock4, int sock6, struct im
 	event_dispatch();
 	_exit(0);
 }
+
+int
+client_tree_cmp(struct client *a, struct client *b)
+{
+	if (a->id == b->id)
+		return 0;
+	else if (a->id < b->id)
+		return -1;
+	else
+		return +1;
+}
+
+SPLAY_GENERATE(client_tree_id, client, entry, client_tree_cmp)