From a2f49f2bb8d25b6e667aa97356604b7fdd5b6e57 Mon Sep 17 00:00:00 2001
From: Oliver Jowett <oliver@mutability.co.uk>
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 ===========================
 //