fwd: Improve `source' and `target' lifecycle management.
authorMark Wooding <mdw@distorted.org.uk>
Sat, 9 Jun 2018 16:28:14 +0000 (17:28 +0100)
committerMark Wooding <mdw@distorted.org.uk>
Sat, 9 Jun 2018 16:28:14 +0000 (17:28 +0100)
  * Add reference counts and functions for handling them to `source' and
    `target'.

  * Use these instead of poking the `destroy' methods directly.

  * Introduce a new `source' method, `shutdown', to disentangle the
    source from the machinery, and use this in `source_killall'.

This fixes a bug in socket sources with a connection limit: if an
endpoint closes during graceful shutdown, then the endpoint tries to
get the dead source to resume listening, and things go south quickly.

Makefile.am
exec.c
file.c
fwd.c
fwd.h
socket.c
source.c
target.c [new file with mode: 0644]

index 055a217..c6bf142 100644 (file)
@@ -68,6 +68,7 @@ fwd_SOURCES           += fwd.c fwd.h
 fwd_SOURCES            += chan.c
 fwd_SOURCES            += endpt.c
 fwd_SOURCES            += source.c
+fwd_SOURCES            += target.c
 
 fwd_SOURCES            += conf.c
 fwd_SOURCES            += scan.c
diff --git a/exec.c b/exec.c
index d41a31d..fc8b679 100644 (file)
--- a/exec.c
+++ b/exec.c
@@ -1004,6 +1004,8 @@ static source *xsource_read(scanner *sc)
     return (0);
   xs = CREATE(xsource);
   xs->s.ops = &xsource_ops;
+  xs->s.ref = 1;
+  xs->s.f = 0;
   xs->s.desc = 0;
   exec_read(&xs->x, sc);
   return (&xs->s);
@@ -1032,18 +1034,12 @@ static void xsource_attach(source *s, scanner *sc, target *t)
   /* --- Create the endpoints --- */
 
   if ((ee = t->ops->create(t, xs->s.desc)) == 0)
-    goto tidy;
+    return;
   if ((e = exec_endpt(&xs->x, xs->s.desc)) == 0) {
     ee->ops->close(ee);
-    goto tidy;
+    return;
   }
   endpt_join(e, ee, xs->s.desc);
-
-  /* --- Dispose of source and target --- */
-
-tidy:
-  t->ops->destroy(t);
-  xsource_destroy(&xs->s);
 }
 
 /* --- @destroy@ --- */
@@ -1060,7 +1056,7 @@ static void xsource_destroy(source *s)
 
 source_ops xsource_ops = {
   "exec",
-  xsource_option, xsource_read, xsource_attach, xsource_destroy
+  xsource_option, xsource_read, xsource_attach, 0, xsource_destroy
 };
 
 /*----- Exec target description -------------------------------------------*/
@@ -1084,6 +1080,7 @@ static target *xtarget_read(scanner *sc)
     return (0);
   xt = CREATE(xtarget);
   xt->t.ops = &xtarget_ops;
+  xt->t.ref = 1;
   exec_read(&xt->x, sc);
   exec_desc(&xt->x, &d);
   xt->t.desc = xstrdup(d.buf);
diff --git a/file.c b/file.c
index 8b4f533..5da247c 100644 (file)
--- a/file.c
+++ b/file.c
@@ -457,6 +457,8 @@ static source *fsource_read(scanner *sc)
     return (0);
   fs = CREATE(fsource);
   fs->s.ops = &fsource_ops;
+  fs->s.ref = 1;
+  fs->s.f = 0;
   fs->s.desc = 0;
   file_read(&fs->f, sc);
   return (&fs->s);
@@ -485,18 +487,12 @@ static void fsource_attach(source *s, scanner *sc, target *t)
   /* --- And then create the endpoints --- */
 
   if ((ee = t->ops->create(t, fs->s.desc)) == 0)
-    goto tidy;
+    return;
   if ((e = file_endpt(&fs->f, fs->s.desc)) == 0) {
     ee->ops->close(ee);
-    goto tidy;
+    return;
   }
   endpt_join(e, ee, fs->s.desc);
-
-  /* --- Dispose of the source and target now --- */
-
-tidy:
-  t->ops->destroy(t);
-  fsource_destroy(&fs->s);
 }
 
 /* --- @destroy@ --- */
@@ -513,7 +509,7 @@ static void fsource_destroy(source *s)
 
 source_ops fsource_ops = {
   "file",
-  fsource_option, fsource_read, fsource_attach, fsource_destroy
+  fsource_option, fsource_read, fsource_attach, 0, fsource_destroy
 };
 
 /*----- File target description -------------------------------------------*/
@@ -537,6 +533,7 @@ static target *ftarget_read(scanner *sc)
     return (0);
   ft = CREATE(ftarget);
   ft->t.ops = &ftarget_ops;
+  ft->t.ref = 1;
   file_read(&ft->f, sc);
   file_desc(&ft->f, &d);
   ft->t.desc = xstrdup(d.buf);
diff --git a/fwd.c b/fwd.c
index 0455bc4..9d170e1 100644 (file)
--- a/fwd.c
+++ b/fwd.c
@@ -152,6 +152,7 @@ void parse(scanner *sc)
       s->ops->attach(s, sc, t);
       if (t->ops->confirm)
        t->ops->confirm(t);
+      source_dec(s); target_dec(t);
     }
 
     /* --- Include configuration from a file --- *
diff --git a/fwd.h b/fwd.h
index f075c53..e295c8b 100644 (file)
--- a/fwd.h
+++ b/fwd.h
@@ -899,6 +899,7 @@ typedef struct endpt_ops {
 
 typedef struct target {
   struct target_ops *ops;
+  unsigned ref;
   char *desc;
 } target;
 
@@ -973,9 +974,13 @@ typedef struct target_ops {
 typedef struct source {
   struct source *next, *prev;
   struct source_ops *ops;
+  unsigned ref;
+  unsigned f;
+#define SF_ACTIVE 1u
   char *desc;
 } source;
 
+
 /* --- Forwarding source operations --- */
 
 typedef struct source_ops {
@@ -1019,6 +1024,18 @@ typedef struct source_ops {
 
   void (*attach)(source */*s*/, scanner */*sc*/, target */*t*/);
 
+  /* --- @shutdown@ --- *
+   *
+   * Arguments:        @source *s@ = pointer to source
+   *
+   * Returns:  ---
+   *
+   * Use:      Deactivates the source so that it won't produce any more
+   *           endpoints.
+   */
+
+  void (*shutdown)(source */*s*/);
+
   /* --- @destroy@ --- *
    *
    * Arguments:        @source *s@ = pointer to source
@@ -1078,6 +1095,28 @@ extern void endpt_killall(void);
 
 extern void endpt_join(endpt */*a*/, endpt */*b*/, const char */*desc*/);
 
+/* --- @target_inc@ --- *
+ *
+ * Arguments:  @target *t@ = pointer to a source
+ *
+ * Returns:    ---
+ *
+ * Use:                Increments a target's refcount.
+ */
+
+extern void target_inc(target */*t*/);
+
+/* --- @target_dec@ --- *
+ *
+ * Arguments:  @target *t@ = pointer to a target
+ *
+ * Returns:    ---
+ *
+ * Use:                Decrements a target's refcount, destroying it if necessary.
+ */
+
+extern void target_dec(target */*t*/);
+
 /* --- @source_add@ --- *
  *
  * Arguments:  @source *s@ = pointer to a source
@@ -1102,6 +1141,28 @@ extern void source_add(source */*s*/);
 
 extern void source_remove(source */*s*/);
 
+/* --- @source_inc@ --- *
+ *
+ * Arguments:  @source *s@ = pointer to a source
+ *
+ * Returns:    ---
+ *
+ * Use:                Increments a source's refcount.
+ */
+
+extern void source_inc(source */*s*/);
+
+/* --- @source_dec@ --- *
+ *
+ * Arguments:  @source *s@ = pointer to a source
+ *
+ * Returns:    ---
+ *
+ * Use:                Decrements a source's refcount, destroying it if necessary.
+ */
+
+extern void source_dec(source */*s*/);
+
 /* --- @source_killall@ --- *
  *
  * Arguments:  ---
index a1a9a55..36e58ea 100644 (file)
--- a/socket.c
+++ b/socket.c
@@ -170,7 +170,7 @@ static void ssept_close(endpt *e)
 {
   ssept *ee = (ssept *)e;
 
-  if (ee->s->o.opt == SOCKOPT_LIMIT) {
+  if ((ee->s->s.f&SF_ACTIVE) && ee->s->o.opt == SOCKOPT_LIMIT) {
     ee->s->o.conn++;
     if (ee->s->o.conn == 1)
       ss_listen(ee->s);
@@ -351,6 +351,8 @@ static source *ssource_read(scanner *sc)
   (void)(conf_prefix(sc, "socket") || conf_prefix(sc, "sk"));
   ss = CREATE(ssource);
   ss->s.ops = &ssource_ops;
+  ss->s.ref = 1;
+  ss->s.f = 0;
   ss->s.desc = 0;
   ss->t = 0;
   ss->a = getaddr(sc, ADDR_SRC);
@@ -447,7 +449,7 @@ static void ss_accept(int fd, unsigned mode, void *p)
        close(ss->r.fd);
        if (ss->a->ops->unbind)
          ss->a->ops->unbind(ss->a);
-       ssource_destroy(&ss->s);
+       source_dec(&ss->s);
        acceptp = 0;
        break;
     }
@@ -511,7 +513,7 @@ fail_1:
   close(fd);
 fail_0:
   ss->o.conn = 0;
-  ssource_destroy(&ss->s);
+  source_dec(&ss->s);
 }
 
 /* --- @attach@ --- */
@@ -522,7 +524,7 @@ static void ssource_attach(source *s, scanner *sc, target *t)
   int fd;
   int opt = 1;
 
-  ss->t = t;
+  ss->t = t; target_inc(t);
 
   /* --- Initialize the description string --- */
 
@@ -563,9 +565,9 @@ static void ssource_attach(source *s, scanner *sc, target *t)
   fw_inc();
 }
 
-/* --- @destroy@ --- */
+/* --- @shutdown@ --- */
 
-static void ssource_destroy(source *s)
+static void ssource_shutdown(source *s)
 {
   ssource *ss = (ssource *)s;
 
@@ -579,10 +581,19 @@ static void ssource_destroy(source *s)
     ss->a->ops->freesrcopts(ss->ao);
   else
     DESTROY(ss->ao);
-  xfree(ss->s.desc);
   ss->a->ops->destroy(ss->a);
-  ss->t->ops->destroy(ss->t);
   source_remove(&ss->s);
+  target_dec(ss->t);
+  fw_dec();
+}
+
+/* --- @destroy@ --- */
+
+static void ssource_destroy(source *s)
+{
+  ssource *ss = (ssource *)s;
+
+  xfree(ss->s.desc);
   DESTROY(ss);
   fw_dec();
 }
@@ -591,7 +602,7 @@ static void ssource_destroy(source *s)
 
 source_ops ssource_ops = {
   "socket",
-  ssource_option, ssource_read, ssource_attach, ssource_destroy
+  ssource_option, ssource_read, ssource_attach, ssource_shutdown, ssource_destroy
 };
 
 /*----- Target definition -------------------------------------------------*/
@@ -627,6 +638,7 @@ static target *starget_read(scanner *sc)
   (void)(conf_prefix(sc, "socket") || conf_prefix(sc, "sk"));
   st = CREATE(starget);
   st->t.ops = &starget_ops;
+  st->t.ref = 1;
   st->a = getaddr(sc, ADDR_DEST);
   if (st->a->ops->inittargopts)
     st->ao = st->a->ops->inittargopts();
index 96b7f81..f515ae2 100644 (file)
--- a/source.c
+++ b/source.c
@@ -45,11 +45,15 @@ static source *sources = 0;
 
 void source_add(source *s)
 {
+  assert(s->ops->shutdown);
+  assert(!(s->f&SF_ACTIVE));
   s->next = sources;
   s->prev = 0;
   if (sources)
     sources->prev = s;
   sources = s;
+  s->f |= SF_ACTIVE;
+  source_inc(s);
 }
 
 /* --- @source_remove@ --- *
@@ -63,12 +67,45 @@ void source_add(source *s)
 
 void source_remove(source *s)
 {
+  assert(s->f&SF_ACTIVE);
   if (s->next)
     s->next->prev = s->prev;
   if (s->prev)
     s->prev->next = s->next;
   else
     sources = s->next;
+  s->f &= ~SF_ACTIVE;
+  source_dec(s);
+}
+
+/* --- @source_inc@ --- *
+ *
+ * Arguments:  @source *s@ = pointer to a source
+ *
+ * Returns:    ---
+ *
+ * Use:                Increments a source's refcount.
+ */
+
+void source_inc(source *s) { s->ref++; }
+
+/* --- @source_dec@ --- *
+ *
+ * Arguments:  @source *s@ = pointer to a source
+ *
+ * Returns:    ---
+ *
+ * Use:                Decrements a source's refcount, destroying it if necessary.
+ */
+
+void source_dec(source *s)
+{
+  assert(s->ref > 0);
+  s->ref--;
+  if (!s->ref) {
+    if (s->f&SF_ACTIVE) s->ops->shutdown(s);
+    s->ops->destroy(s);
+  }
 }
 
 /* --- @source_killall@ --- *
@@ -86,7 +123,7 @@ void source_killall(void)
   while (s) {
     source *ss = s;
     s = s->next;
-    ss->ops->destroy(ss);
+    ss->ops->shutdown(ss);
   }
   sources = 0;
 }
diff --git a/target.c b/target.c
new file mode 100644 (file)
index 0000000..5590761
--- /dev/null
+++ b/target.c
@@ -0,0 +1,60 @@
+/* -*-c-*-
+ *
+ * Common functionality for targets
+ *
+ * (c) 2018 Straylight/Edgeware
+ */
+
+/*----- Licensing notice --------------------------------------------------*
+ *
+ * This file is part of the `fwd' port forwarder.
+ *
+ * `fwd' is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * `fwd' is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with `fwd'.  If not, write to the Free Software Foundation,
+ * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+/*----- Header files ------------------------------------------------------*/
+
+#include "fwd.h"
+
+/*----- Main code ---------------------------------------------------------*/
+
+/* --- @target_inc@ --- *
+ *
+ * Arguments:  @target *t@ = pointer to a source
+ *
+ * Returns:    ---
+ *
+ * Use:                Increments a target's refcount.
+ */
+
+void target_inc(target *t) { t->ref++; }
+
+/* --- @target_dec@ --- *
+ *
+ * Arguments:  @target *t@ = pointer to a target
+ *
+ * Returns:    ---
+ *
+ * Use:                Decrements a target's refcount, destroying it if necessary.
+ */
+
+void target_dec(target *t)
+{
+  assert(t->ref > 0);
+  t->ref--;
+  if (!t->ref) t->ops->destroy(t);
+}
+
+/*----- That's all, folks -------------------------------------------------*/