From a94ef43ad751b60dbc8cefe1fe1e86a1065c2e0c Mon Sep 17 00:00:00 2001 From: Egor Date: Sat, 21 Oct 2023 14:19:02 +0300 Subject: [PATCH] handle write errors and connection closes more gracefully --- src/common.h | 3 ++ src/main.c | 86 +++++++++++++++++++++++++++++++-------------- src/net.c | 98 +++++++++++++++++++++++++++++++++++----------------- 3 files changed, 130 insertions(+), 57 deletions(-) diff --git a/src/common.h b/src/common.h index f857071..a9b55b2 100644 --- a/src/common.h +++ b/src/common.h @@ -58,6 +58,8 @@ struct client_data { unsigned want_write_for_write :1; unsigned want_read_for_close :1; unsigned want_write_for_close :1; + unsigned dying :1; + unsigned write_error :1; }; struct channel { char *key; @@ -93,6 +95,7 @@ extern struct trie_node *client_map; extern struct trie_node *channel_map; extern struct trie_node *autojoin; extern struct client_data *clients; +extern struct client_data *dyingclients; extern fd_set readfds; extern struct sockaddr_in serverAddrV4; extern struct sockaddr_in6 serverAddrV6; diff --git a/src/main.c b/src/main.c index f3cbb0c..16d96a8 100644 --- a/src/main.c +++ b/src/main.c @@ -3,13 +3,15 @@ #include "irc.h" #include +static int is_running = 1; + #ifndef _WIN32 #include -static sig_atomic_t is_running = 1; +static volatile sig_atomic_t is_siginted = 0; static void on_sigint(int signum) { (void)signum; - is_running = 0; + is_siginted = 1; } static void set_handler() { @@ -25,12 +27,14 @@ static void set_handler() perror("sigaction"); } #else -static int is_running = 1; +static int is_siginted_once = 0; +static volatile int is_siginted = 0; static SOCKET self_sock = INVALID_SOCKET; static BOOL WINAPI on_sigint(DWORD dwCtrlType) { - if ((dwCtrlType == CTRL_C_EVENT || dwCtrlType == CTRL_CLOSE_EVENT) && is_running) { - is_running = 0; + if ((dwCtrlType == CTRL_C_EVENT || dwCtrlType == CTRL_CLOSE_EVENT) && !is_siginted_once) { + is_siginted_once = 1; + is_siginted = 1; char x = 0; if (self_sock != INVALID_SOCKET) { send(self_sock, &x, 0, 0); // FIXME: this could EAGAIN in theory @@ -71,7 +75,7 @@ err: #endif // Client linked list -struct client_data *clients = NULL; +struct client_data *clients = NULL, *dyingclients = NULL; // FDSET List fd_set readfds, writefds; @@ -312,7 +316,22 @@ int main(int argc, char *argv[]) } // Main loop - while(is_running){ + while (is_running || dyingclients) { + if (is_siginted) { + fprintf(stderr, "[PatchyIRCD]: Cleaning up...\n"); + for (int i = 0; i < listener_count; i++) + closesocket(listeners[i].sock); + listener_count = 0; + for (struct client_data *cl = clients; cl;) { + struct client_data *next = cl->next; + killConnection(cl, 0); + cl = next; + } + is_running = 0; + is_siginted = 0; + if (!dyingclients) + break; + } FD_ZERO(&readfds); FD_ZERO(&writefds); // Add listening SOCKETS to FDSET @@ -323,9 +342,15 @@ int main(int argc, char *argv[]) FD_SET(self_sock, &readfds); #endif for (struct client_data *cl = clients; cl; cl = cl->next) { - if (!cl->want_write_for_close) + if (!cl->want_write_for_read || cl->want_read_for_write) FD_SET(cl->FD, &readfds); - if (cl->want_write_for_read || cl->want_write_for_write || cl->want_write_for_close) + if (cl->want_write_for_read || cl->want_write_for_write) + FD_SET(cl->FD, &writefds); + } + for (struct client_data *cl = dyingclients; cl; cl = cl->next) { + if (cl->want_read_for_write || cl->want_read_for_close) + FD_SET(cl->FD, &readfds); + if (cl->want_write_for_write || cl->want_write_for_close) FD_SET(cl->FD, &writefds); } @@ -340,6 +365,9 @@ int main(int argc, char *argv[]) for (struct client_data *cl = clients; cl; cl = cl->next) if (highestFD < cl->FD) highestFD = cl->FD; + for (struct client_data *cl = dyingclients; cl; cl = cl->next) + if (highestFD < cl->FD) + highestFD = cl->FD; #endif // Wait until activity on sockets if(select(highestFD+1,&readfds,&writefds,NULL,NULL) == -1) { @@ -357,36 +385,42 @@ int main(int argc, char *argv[]) } #endif - // Accept new connections + /* Accept new connections */ for (int i = 0; i < listener_count; i++) if (FD_ISSET(listeners[i].sock, &readfds)) accept_from_sock(&listeners[i]); - // Respond to already connected clients + /* Respond to already connected clients */ for (struct client_data *cl = clients; cl;) { struct client_data *next = cl->next; int isr = FD_ISSET(cl->FD, &readfds); int isw = FD_ISSET(cl->FD, &writefds); - if (!cl->want_read_for_close && !cl->want_write_for_close) - if (isr || (cl->want_write_for_read && isw)) - recvProc(cl); + /* NOTE: recvProc can kill the client, so writing should be done first. */ if ((cl->want_read_for_write && isr) || (cl->want_write_for_write && isw)) sendClient(cl, 0, 0); - if ((cl->want_read_for_close && isr) || (cl->want_write_for_close && isw)) - killConnectionPart2(cl); + if (cl->write_error) + killConnection(cl, 0); + else if (cl->want_write_for_read ? isw : isr) + recvProc(cl); + cl = next; + } + /* Close dying clients */ + for (struct client_data *cl = dyingclients; cl;) { + struct client_data *next = cl->next; + int isr = FD_ISSET(cl->FD, &readfds); + int isw = FD_ISSET(cl->FD, &writefds); + if (cl->wbufLen) + if ((cl->want_read_for_write && isr) || (cl->want_write_for_write && isw)) + sendClient(cl, 0, 0); + if (!cl->wbufLen) { + if (!cl->want_read_for_close && !cl->want_write_for_close) + killConnectionPart2(cl); + else if ((cl->want_read_for_close && isr) || (cl->want_write_for_close && isw)) + killConnectionPart2(cl); + } cl = next; } } - fprintf(stderr, "[PatchyIRCD]: Cleaning up...\n"); - for (struct client_data *cl = clients; cl;) { - struct client_data *next = cl->next; - killConnection(cl, 0); - /* FIXME: who is going to wait for these connections to actually get - * killed? */ - cl = next; - } - for (int i = 0; i < listener_count; i++) - closesocket(listeners[i].sock); net_cleanup(); #ifdef HAS_TLS tls_free(tls_ctx); diff --git a/src/net.c b/src/net.c index e17a167..968935b 100644 --- a/src/net.c +++ b/src/net.c @@ -9,24 +9,55 @@ struct client_data *allocClient() void killConnection(struct client_data *cl, int byChoice) { - if (!cl) return; + if (!cl) + return; + if (cl->dying) + return; + cl->dying = 1; /* NOTE: this recursion guard isn't necessary, but it's there just in case */ if (cl->peername) fprintf(stderr, "Client disconnected from Socket: %"PRISocket", IP: %s PORT: %d\n", cl->FD, cl->peername, cl->peerport); - if(!byChoice && cl->nick && cl->username) { + if (!byChoice && cl->nick && cl->username) { msg_quit(cl, "", "Has left the building (Forced)"); sendbuf_visible(cl); } - // Send error to client informing them that they will be disconnected + /* Send error to client informing them that they will be disconnected */ msg_error("Closing Link"); sendbuf(cl); + /* Move the client to the dyingclients chain */ + for (struct client_data **next = &clients; *next; next = &(*next)->next) { + if (*next == cl) { + *next = cl->next; + break; + } + } + cl->next = dyingclients; + dyingclients = cl; + /* Part all channels */ + struct trie_find *f = NULL; + if (trie_findfirst(&f, &cl->channels) == 1) + do { + struct channel *chan = f->n->value; + channel_part(cl, chan, NULL, NULL); + } while(trie_findfirst(&f, &cl->channels) == 1); + /* Get rid of the pending invites */ + if (cl->key) { + trie_set(&client_map, cl->key, NULL, NULL); + struct trie_find *f = NULL; + if (trie_findfirst(&f, &cl->invites) == 1) + do { + struct channel *chan = f->n->value; + trie_set(&chan->invitees, cl->key, NULL, NULL); + } while(trie_findnext(&f) == 1); + } killConnectionPart2(cl); } void killConnectionPart2(struct client_data *cl) { - /* FIXME: we have to flush existing data somehow */ - /* A better implementation would be to keep a separate list of disconnected - * clients. This way we can do all PARTing right away and only concern - * ourselves with tls_write/tls_close. */ + if (cl->want_read_for_write || cl->want_write_for_write) { + /* We're still waiting for a pending write. + * This could be the case when this is called from killConnection. */ + return; + } cl->want_read_for_close = 0; cl->want_write_for_close = 0; if (cl->FD != INVALID_SOCKET) { @@ -51,28 +82,14 @@ void killConnectionPart2(struct client_data *cl) shutdown(cl->FD, SHUT_RDWR); closesocket(cl->FD); } - for (struct client_data **next = &clients; *next; next = &(*next)->next) { + /* Remove the client from the dyingclients chain */ + for (struct client_data **next = &dyingclients; *next; next = &(*next)->next) { if (*next == cl) { *next = cl->next; break; } } - struct trie_find *f = NULL; - if (trie_findfirst(&f, &cl->channels) == 1) - do { - struct channel *chan = f->n->value; - channel_part(cl, chan, NULL, NULL); - } while(trie_findfirst(&f, &cl->channels) == 1); - free(f); - if (cl->key) { - trie_set(&client_map, cl->key, NULL, NULL); - struct trie_find *f = NULL; - if (trie_findfirst(&f, &cl->invites) == 1) - do { - struct channel *chan = f->n->value; - trie_set(&chan->invitees, cl->key, NULL, NULL); - } while(trie_findnext(&f) == 1); - } + /* Free all the remaining data */ free(cl->nick); free(cl->key); free(cl->username); @@ -140,16 +157,16 @@ void recvProc(struct client_data *cl) { if(!x) break; packetReturn = parsePacket(cl); // Client QUIT - if(packetReturn == 1) { + if (packetReturn == 1 || cl->write_error) return; - } memmove(cl->buf, x+1, cl->buf + cl->bufLen-(x+1)); cl->bufLen -= x+1 - cl->buf; } } } -/* returns the count of bytes which were not sent */ +/* returns the count of bytes which were not sent, or -1 on error. + * updates want_read_for_write and want_write_for_write */ static int sendClientRaw(struct client_data *cl, const char *buf, size_t len) { cl->want_read_for_write = 0; @@ -157,13 +174,12 @@ static int sendClientRaw(struct client_data *cl, const char *buf, size_t len) if (cl->FD == (SOCKET)INVALID_SOCKET) return 0; fprintf(stderr, "SERVER %"PRISocket": %.*s", cl->FD, (int)len, (char*)buf); - /* FIXME: error handling */ #ifdef HAS_TLS if (cl->cctx) { while (len) { int rv = tls_write(cl->cctx, buf, len); if (rv == 0 || rv == -1) { - break; + return -1; } else if (rv == TLS_WANT_POLLIN) { cl->want_read_for_write = 1; return len; @@ -180,13 +196,13 @@ static int sendClientRaw(struct client_data *cl, const char *buf, size_t len) while (len) { int rv = send(cl->FD, buf, len, 0); if (!rv) - break; + return -1; if (rv == -1) { if (net_wouldblock()) { cl->want_write_for_write = 1; return len; } else { - break; + return -1; } } buf += rv; @@ -198,6 +214,8 @@ static int sendClientRaw(struct client_data *cl, const char *buf, size_t len) void sendClient(struct client_data *cl, const char *buf, size_t len) { + if (cl->write_error) + return; if (cl->wbufLen + len > sizeof(cl->wbuf)) { /* Buffer overrun */ /* FIXME: probably should terminate the connection at this point */ @@ -207,6 +225,19 @@ void sendClient(struct client_data *cl, const char *buf, size_t len) if (!cl->wbufLen) { if (len) { int left = sendClientRaw(cl, buf, len); + if (left == -1) { + /* Write error happened, discard the buffer and close the + * connection. sendClient is used from a ton of places, + * including killConnection and processPacket. The former will + * recurse into this function, and the latter assumes that this + * function will not cause the client to go away. For this + * reason we can't just use killConnection. Instead we just + * mark the client with a flag and make all the following + * writes instantly succeed. + */ + cl->write_error = 1; + return; + } memcpy(cl->wbuf, buf+len-left, left); cl->wbufLen = left; } @@ -217,6 +248,11 @@ void sendClient(struct client_data *cl, const char *buf, size_t len) cl->wbufLen += len; } int left = sendClientRaw(cl, cl->wbuf, cl->wbufLen); + if (left == -1) { + cl->wbufLen = 0; + cl->write_error = 1; + return; + } memmove(cl->wbuf, cl->wbuf+cl->wbufLen-left, left); cl->wbufLen = left; } -- 2.56.0