From b7d9dc14eb560b1d19f506610535815f29085579 Mon Sep 17 00:00:00 2001 From: Dirk Engling Date: Sat, 31 Oct 2026 19:43:36 +0100 Subject: Make the dynamic accesslist readers release their line buffer and properly exit --- Makefile | 2 +- ot_accesslist.c | 46 ++++++++++++++++++++++--- tests/integration/test_dynamic_accesslist.py | 50 ++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 6 deletions(-) create mode 100644 tests/integration/test_dynamic_accesslist.py 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") -- cgit v1.3