From: Mark Wooding <mdw@distorted.org.uk>
Date: Sun, 14 Dec 2008 22:03:21 +0000 (+0000)
Subject: server/admin: Fix client destruction some more.
X-Git-Tag: 1.0.0pre8~45
X-Git-Url: https://git.distorted.org.uk/~mdw/tripe/commitdiff_plain/94593d1b0c15761b829f98a804b2ef42a4a84b0a

server/admin: Fix client destruction some more.

It's possible that finally destroying a client can kill others.  For
example, if the second client (a) has sent EOF, and (b) has a background
command outstanding with the first; then when the first sends EOF, the
second gets taken down too.

Unfortunately, what actually happens in this case is that the newly
defunct clients get put on the dead list -- and then ignored because the
dead list is silently killed at the end of a_destroypending.  Fix by
clearing the list at the top of a_destroypending, and doing the whole
job repeatedly until there are no more cascades.

The change is mostly indenting a loop: it looks less scary with -b.
---

diff --git a/server/admin.c b/server/admin.c
index 1a4d2a45..c4433e45 100644
--- a/server/admin.c
+++ b/server/admin.c
@@ -1913,61 +1913,66 @@ static void acmd_help(admin *a, unsigned ac, char *av[])
 
 static void a_destroypending(void)
 {
-  admin *a, *aa;
+  admin *a, *aa, *head;
   admin_bgop *bg, *bbg;
   admin_service *svc, *ssvc;
 
-  /* --- Destroy connections marked as pending --- */
+  /* --- Destroy connections marked as pending --- *
+   *
+   * Slightly messy.  Killing clients may cause others to finally die.  Make
+   * sure that they can be put on the list without clobbering anything or
+   * getting lost.
+   */
 
-  for (a = a_dead; a; a = aa) {
-    aa = a->next;
-    assert(a->f & AF_DEAD);
+  while (a_dead) {
+    head = a_dead;
+    a_dead = 0;
+    for (a = head; a; a = aa) {
+      aa = a->next;
+      assert(a->f & AF_DEAD);
 
-    /* --- Report what we're doing --- */
+      /* --- Report what we're doing --- */
 
-    T( trace(T_ADMIN, "admin: completing destruction of connection %u",
-	     a->seq); )
+      T( trace(T_ADMIN, "admin: completing destruction of connection %u",
+	       a->seq); )
 
-    /* --- If this is the foreground client then shut down --- */
+      /* --- If this is the foreground client then shut down --- */
 
-    if (a->f & AF_FOREGROUND) {
-      T( trace(T_ADMIN, "admin: foreground client quit: shutting down"); )
-      a_warn("SERVER", "quit", "foreground-eof", A_END);
-      a_quit();
-    }
+      if (a->f & AF_FOREGROUND) {
+	T( trace(T_ADMIN, "admin: foreground client quit: shutting down"); )
+	a_warn("SERVER", "quit", "foreground-eof", A_END);
+	a_quit();
+      }
 
-    /* --- Abort any background jobs in progress --- */
+      /* --- Abort any background jobs in progress --- */
 
-    for (bg = a->bg; bg; bg = bbg) {
-      bbg = bg->next;
-      bg->cancel(bg);
-      if (bg->tag) xfree(bg->tag);
-      xfree(bg);
-    }
+      for (bg = a->bg; bg; bg = bbg) {
+	bbg = bg->next;
+	bg->cancel(bg);
+	if (bg->tag) xfree(bg->tag);
+	xfree(bg);
+      }
 
-    /* --- Release services I hold, and abort pending jobs --- */
+      /* --- Release services I hold, and abort pending jobs --- */
 
-    for (svc = a->svcs; svc; svc = ssvc) {
-      ssvc = svc->next;
-      a_svcrelease(svc);
-    }
-    a_jobtablefinal(&a->j);
+      for (svc = a->svcs; svc; svc = ssvc) {
+	ssvc = svc->next;
+	a_svcrelease(svc);
+      }
+      a_jobtablefinal(&a->j);
 
-    /* --- Close file descriptors and selectory --- */
+      /* --- Close file descriptors and selectory --- */
 
-    selbuf_destroy(&a->b);
-    if (a->b.reader.fd != a->w.fd) close(a->b.reader.fd);
-    close(a->w.fd);
-    if (a_stdin == a) a_stdin = 0;
+      selbuf_destroy(&a->b);
+      if (a->b.reader.fd != a->w.fd) close(a->b.reader.fd);
+      close(a->w.fd);
+      if (a_stdin == a) a_stdin = 0;
 
-    /* --- Done --- */
+      /* --- Done --- */
 
-    DESTROY(a);
+      DESTROY(a);
+    }
   }
-
-  /* --- All pending destruction completed --- */
-
-  a_dead = 0;
 }
 
 /* --- @a_destroy@ --- *