3 files changed, 92 insertions, 6 deletions

diff --git a/Makefile b/Makefile
index 61eb872..dbc6166 100644
--- a/Makefile
+++ b/Makefile
@@ -136,7 +136,7 @@ check-sanitize: $(TESTBIN).sanitize
 PYTHON?=python3
 # The client timeout is cut right down so that tests can reach the expiry
 # sweep. tests/integration/test_expiry.py mirrors these two numbers.
-FEATURES_integration=$(FEATURES) -DWANT_IP_FROM_QUERY_STRING -DWANT_ACCESSLIST_BLACK -DWANT_KEEPALIVE -DOT_CLIENT_TIMEOUT=5 -DOT_CLIENT_TIMEOUT_CHECKINTERVAL=1
+FEATURES_integration=$(FEATURES) -DWANT_IP_FROM_QUERY_STRING -DWANT_ACCESSLIST_BLACK -DWANT_DYNAMIC_ACCESSLIST -DWANT_KEEPALIVE -DOT_CLIENT_TIMEOUT=5 -DOT_CLIENT_TIMEOUT_CHECKINTERVAL=1
 CFLAGS_integration=$(CFLAGS) $(OPTS_debug) $(FEATURES_integration) -fsanitize=address,undefined -fno-omit-frame-pointer
 LDFLAGS_integration=$(LDFLAGS) -fsanitize=address,undefined
 
diff --git a/ot_accesslist.c b/ot_accesslist.c
index e8e0476..df084b8 100644
--- a/ot_accesslist.c
+++ b/ot_accesslist.c
@@ -273,7 +273,8 @@ static void *accesslist_worker(void *args) {
 }
 
 #ifdef WANT_DYNAMIC_ACCESSLIST
-static pthread_t thread_adder_id, thread_deleter_id;
+static pthread_t   thread_adder_id, thread_deleter_id;
+static _Atomic int adder_left, deleter_left;
 static void     *accesslist_adddel_worker(const char *fifoname, ot_accesslist *_Atomic *adding_to, ot_accesslist *_Atomic *removing_from) {
   struct stat st;
 
@@ -297,6 +298,9 @@ static void     *accesslist_adddel_worker(const char *fifoname, ot_accesslist *_
     ssize_t linelen;
 
     if (!fifo) {
+      /* Woken on purpose: the loop condition above decides what happens next */
+      if (errno == EINTR)
+        continue;
       fprintf(stderr, "Error when reading dynamic accesslists: Couldn't open FIFO at %s, error: %s\n", fifoname, strerror(errno));
       return NULL;
     }
@@ -324,8 +328,14 @@ static void     *accesslist_adddel_worker(const char *fifoname, ot_accesslist *_
           ptrdiff_t      off            = exactmatch - (*removing_from)->list;
           ot_accesslist *accesslist_new = accesslist_make(*removing_from, (*removing_from)->size - 1);
           if (accesslist_new) {
-            memcpy(accesslist_new->list, (*removing_from)->list, sizeof(ot_hash) * off);
-            memcpy(accesslist_new->list + off, (*removing_from)->list + off + 1, sizeof(ot_hash) * ((*removing_from)->size - off - 1));
+            size_t after = (*removing_from)->size - off - 1;
+
+            /* Removing the only entry leaves a list with no room at all, and
+               memcpy may not be handed that even for nothing */
+            if (off)
+              memcpy(accesslist_new->list, (*removing_from)->list, sizeof(ot_hash) * off);
+            if (after)
+              memcpy(accesslist_new->list + off, (*removing_from)->list + off + 1, sizeof(ot_hash) * after);
             (*removing_from)->base = time_now_minutes();
             *removing_from = accesslist_new;
           }
@@ -360,6 +370,7 @@ static void     *accesslist_adddel_worker(const char *fifoname, ot_accesslist *_
       pthread_mutex_unlock(&g_accesslist_mutex);
     }
 
+    free(line);
     fclose(fifo);
   }
   return NULL;
@@ -367,11 +378,30 @@ static void     *accesslist_adddel_worker(const char *fifoname, ot_accesslist *_
 
 static void *accesslist_adder_worker(void *args) {
   (void)args;
-  return accesslist_adddel_worker(g_accesslist_pipe_add, &g_accesslist_add, &g_accesslist_delete);
+  accesslist_adddel_worker(g_accesslist_pipe_add, &g_accesslist_add, &g_accesslist_delete);
+  atomic_store(&adder_left, 1);
+  return NULL;
 }
 static void *accesslist_deleter_worker(void *args) {
   (void)args;
-  return accesslist_adddel_worker(g_accesslist_pipe_delete, &g_accesslist_delete, &g_accesslist_add);
+  accesslist_adddel_worker(g_accesslist_pipe_delete, &g_accesslist_delete, &g_accesslist_add);
+  atomic_store(&deleter_left, 1);
+  return NULL;
+}
+
+/* These sit in a blocking open or read on a FIFO, so they are woken the way
+   the other workers are, and the signal is repeated because it only
+   interrupts a call already under way. The pipe name is what says whether
+   the thread was ever started. */
+static void accesslist_stop_fifo_worker(pthread_t thread, _Atomic int *left, const char *pipe_name) {
+  if (!pipe_name)
+    return;
+
+  while (!atomic_load(left)) {
+    pthread_kill(thread, OT_SIGNAL_WAKE);
+    usleep(1000);
+  }
+  pthread_join(thread, NULL);
 }
 #endif
 
@@ -392,6 +422,12 @@ void accesslist_deinit(void) {
   pthread_kill(thread_id, SIGHUP);
   pthread_join(thread_id, NULL);
 
+#ifdef WANT_DYNAMIC_ACCESSLIST
+  /* Before the lists they are reading are freed below */
+  accesslist_stop_fifo_worker(thread_adder_id, &adder_left, g_accesslist_pipe_add);
+  accesslist_stop_fifo_worker(thread_deleter_id, &deleter_left, g_accesslist_pipe_delete);
+#endif
+
   pthread_mutex_lock(&g_accesslist_mutex);
 
   g_accesslist = accesslist_free(g_accesslist);
diff --git a/tests/integration/test_dynamic_accesslist.py b/tests/integration/test_dynamic_accesslist.py
new file mode 100644
index 0000000..7fa5fde
--- /dev/null
+++ b/tests/integration/test_dynamic_accesslist.py
@@ -0,0 +1,50 @@
+"""Hashes added to and removed from the blacklist while the tracker runs.
+
+The two FIFOs are read by threads of their own, which is also what makes
+this the test that drives those threads through a shutdown.
+"""
+
+import os
+import time
+import unittest
+
+from harness import TrackerTestCase
+
+BLOCKED = bytes(range(1, 21))
+
+
+def as_hex(info_hash):
+    return "".join("%02x" % b for b in info_hash)
+
+
+class DynamicAccesslist(TrackerTestCase):
+    config = ("access.blacklist {dir}/blacklist\n"
+              "access.fifo_add {dir}/add.fifo\n"
+              "access.fifo_delete {dir}/delete.fifo\n")
+    files = {"blacklist": b""}
+
+    def feed(self, name, info_hash):
+        """Writing to the FIFO blocks until the reader has it, so this also
+           tells us the thread is alive."""
+        with open(os.path.join(self.tracker.dir, name), "w") as fifo:
+            fifo.write("%s added by a test\n" % as_hex(info_hash))
+
+    def wait_for(self, info_hash, blocked, patience=5.0):
+        """The reader thread picks the hash up on its own schedule, so this
+           waits for the answer to change rather than assuming it already has."""
+        deadline = time.time() + patience
+        while time.time() < deadline:
+            answer = self.tracker.announce(info_hash)
+            if (b"failure reason" in answer) == blocked:
+                return True
+            time.sleep(0.05)
+        return False
+
+    def test_a_hash_can_be_blocked_and_released_while_running(self):
+        self.assertNotIn(b"failure reason", self.tracker.announce(BLOCKED))
+
+        self.feed("add.fifo", BLOCKED)
+        self.assertTrue(self.wait_for(BLOCKED, blocked=True), "the hash was never blocked")
+
+        self.feed("delete.fifo", BLOCKED)
+        self.assertTrue(self.wait_for(BLOCKED, blocked=False), "the hash was never released")