commit 207b3e80d867693ff74cf99c84f7dd41386adba1 from: Omar Polo date: Thu Oct 07 11:20:34 2021 UTC 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.) commit - 4cd25209651f224be8c34d6006ef689963ce37d5 commit + 207b3e80d867693ff74cf99c84f7dd41386adba1 blob - 43c684fec0d931c7f6845d1a37fb8ee624a2db59 blob + 4a8befc7db5c209d20eea8e4a812b0cb8b5ee6dc --- gmid.h +++ gmid.h @@ -20,6 +20,7 @@ #include "config.h" #include +#include #include #include @@ -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)