From a2f49f2bb8d25b6e667aa97356604b7fdd5b6e57 Mon Sep 17 00:00:00 2001 From: Oliver Jowett Date: Thu, 2 Oct 2014 21:06:10 +0100 Subject: [PATCH] Fix a crash when an outbound client is closed in response to reading a remote message. When we read from some client A, we may end up forwarding a message to other clients. If we forward to some client B and there is a write error, then we close B and remove it from the client list. However, if before this happened A->next == B, then the read loop will still be holding on to a pointer to B, and we crash. As it's unpredictable what clients could be closed at what point, the simplest approach is to retain closed clients in the list until we are at a point where we know there are no stray pointers on stack, and only then modify the list. This also simplifies anything that has to loop over clients, as it doesn't need to worry about the current client being freed under it. --- dump1090.c | 1 + dump1090.h | 1 + net_io.c | 56 +++++++++++++++++++++++++++--------------------------- 3 files changed, 30 insertions(+), 28 deletions(-) diff --git a/dump1090.c b/dump1090.c index 7a7addd..3d5ff46 100644 --- a/dump1090.c +++ b/dump1090.c @@ -490,6 +490,7 @@ void showCopyright(void) { void backgroundTasks(void) { if (Modes.net) { modesReadFromClients(); + modesNetCleanup(); } // If Modes.aircrafts is not NULL, remove any stale aircraft diff --git a/dump1090.h b/dump1090.h index 77584bb..5734da5 100644 --- a/dump1090.h +++ b/dump1090.h @@ -454,6 +454,7 @@ void modesReadFromClients (void); void modesSendAllClients (int service, void *msg, int len); void modesQueueOutput (struct modesMessage *mm); void modesReadFromClient(struct client *c, char *sep, int(*handler)(struct client *, char *)); +void modesNetCleanup (void); #ifdef __cplusplus } diff --git a/net_io.c b/net_io.c index 2b0ed9a..ff1b187 100644 --- a/net_io.c +++ b/net_io.c @@ -145,23 +145,11 @@ struct client * modesAcceptClients(void) { // On error free the client, collect the structure, adjust maxfd if needed. // void modesFreeClient(struct client *c) { + // Clean up, but defer removing from the list until modesNetCleanup(). + // This is because there may be stackframes still pointing at this + // client (unpredictably: reading from client A may cause client B to + // be freed) - // Unhook this client from the linked list of clients - struct client *p = Modes.clients; - if (p) { - if (p == c) { - Modes.clients = c->next; - } else { - while ((p) && (p->next != c)) { - p = p->next; - } - if (p) { - p->next = c->next; - } - } - } - - // It's now safe to remove this client close(c->fd); if (c->service == Modes.sbsos) { if (Modes.stat_sbs_connections) Modes.stat_sbs_connections--; @@ -174,7 +162,9 @@ void modesFreeClient(struct client *c) { if (Modes.debug & MODES_DEBUG_NET) printf("Closing client %d\n", c->fd); - free(c); + // mark it as inactive and ready to be freed + c->fd = -1; + c->service = -1; } // //========================================================================= @@ -182,12 +172,9 @@ void modesFreeClient(struct client *c) { // Send the specified message to all clients listening for a given service // void modesSendAllClients(int service, void *msg, int len) { - struct client *c = Modes.clients; - - while (c) { - // Read next before servicing client incase the service routine deletes the client! - struct client *next = c->next; + struct client *c; + for (c = Modes.clients; c; c = c->next) { if (c->service == service) { #ifndef _WIN32 int nwritten = write(c->fd, msg, len); @@ -198,7 +185,6 @@ void modesSendAllClients(int service, void *msg, int len) { modesFreeClient(c); } } - c = next; } } // @@ -944,10 +930,7 @@ void modesReadFromClients(void) { struct client *c = modesAcceptClients(); - while (c) { - // Read next before servicing client incase the service routine deletes the client! - struct client *next = c->next; - + for (; c; c = c->next) { if (c->service == Modes.ris) { modesReadFromClient(c,"\n",decodeHexMessage); } else if (c->service == Modes.bis) { @@ -955,9 +938,26 @@ void modesReadFromClients(void) { } else if (c->service == Modes.https) { modesReadFromClient(c,"\r\n\r\n",handleHTTPRequest); } - c = next; } } + +// +// Perform cleanup of any recently-closed clients. +// +void modesNetCleanup(void) { + struct client *c, **prev; + + for (prev = &Modes.clients, c = *prev; c; c = *prev) { + if (c->fd == -1) { + // Recently closed, prune from list + *prev = c->next; + free(c); + } else { + prev = &c->next; + } + } + } + // // =============================== Network IO =========================== //