factor out accesslist parser to fix a potential out of bound read and add tests to the accesslist unit

7 files changed, 187 insertions, 33 deletions

diff --git a/Makefile b/Makefile
index a6b0b9b..e96269f 100644
--- a/Makefile
+++ b/Makefile
@@ -105,10 +105,13 @@ proxy.debug: $(OBJECTS_proxy_debug) $(HEADERS)
 # has to. tests/stubs.c supplies the few symbols those modules reference but
 # the tests never reach.
 TESTBIN=tests/runner
-TEST_SOURCES=tests/runner.c tests/test.c tests/stubs.c tests/t_scan_urlencoded_query.c tests/t_iovec.c tests/t_vector.c tests/t_random.c tests/t_hex.c
-TEST_UNITS=scan_urlencoded_query.c ot_iovec.c ot_vector.c ot_random.c ot_hex.c
+TEST_SOURCES=tests/runner.c tests/test.c tests/stubs.c tests/t_scan_urlencoded_query.c tests/t_iovec.c tests/t_vector.c tests/t_random.c tests/t_hex.c tests/t_accesslist.c
+TEST_UNITS=scan_urlencoded_query.c ot_iovec.c ot_vector.c ot_random.c ot_hex.c ot_accesslist.c
 TEST_HEADERS=tests/test.h $(HEADERS)
-CFLAGS_test = $(CFLAGS) -I. -g -O1 $(FEATURES)
+# Tests reach code that sits behind feature flags, so the units under test are
+# built with those features enabled. A test binary is not a production binary.
+FEATURES_test=$(FEATURES) -DWANT_ACCESSLIST_BLACK
+CFLAGS_test = $(CFLAGS) -I. -g -O1 $(FEATURES_test)
 
 $(TESTBIN): $(TEST_SOURCES) $(TEST_UNITS) $(TEST_HEADERS)
         $(CC) -o $@ $(CFLAGS_test) $(TEST_SOURCES) $(TEST_UNITS)
diff --git a/ot_accesslist.c b/ot_accesslist.c
index ee6adb2..9b0024c 100644
--- a/ot_accesslist.c
+++ b/ot_accesslist.c
@@ -98,12 +98,33 @@ static void accesslist_clean(ot_accesslist *accesslist) {
   }
 }
 
-/* Read initial access list */
+/* A hash needs forty bytes, and all but a final one are followed by at least
+   one separator, so n hashes need at least 41n - 1 bytes. (len + 1) / 41 is
+   therefore an exact upper bound on what a buffer of len bytes can hold. */
+size_t accesslist_parse_hashes(ot_hash *dest, size_t capacity, const char *buf, size_t len) {
+  const char *end   = buf + len;
+  const char *offs  = buf;
+  size_t      count = 0;
+
+  while (count < capacity && end - offs >= 40) {
+    if (hex_tohash(dest[count], offs)) {
+      offs += 40;
+
+      if (offs == end || !hex_isdigit((unsigned char)*offs))
+        ++count;
+    }
+
+    /* Find start of next line */
+    while (offs < end && *(offs++) != '\n') {}
+  }
+
+  return count;
+}
+
 static void accesslist_readfile(void) {
   ot_accesslist *accesslist_new;
-  ot_hash       *info_hash;
-  const char    *map, *map_end, *read_offs;
-  size_t         maplen;
+  const char    *map;
+  size_t         maplen, capacity, found;
 
   if ((map = mmap_read(g_accesslist_filename, &maplen)) == NULL) {
     char *wd = getcwd(NULL, 0);
@@ -112,42 +133,24 @@ static void accesslist_readfile(void) {
     return;
   }
 
-  /* You need at least 41 bytes to pass an info_hash, make enough room
-     for the maximum amount of them */
-  accesslist_new = accesslist_make(g_accesslist, maplen / 41);
+  /* Make room for the most info_hashes this file could possibly hold */
+  capacity       = (maplen + 1) / 41;
+  accesslist_new = accesslist_make(g_accesslist, capacity);
   if (!accesslist_new) {
-    fprintf(stderr, "Warning: Not enough memory to allocate %zd bytes for accesslist buffer. May succeed later.\n", (maplen / 41) * 20);
+    fprintf(stderr, "Warning: Not enough memory to allocate %zd bytes for accesslist buffer. May succeed later.\n", capacity * sizeof(ot_hash));
     mmap_unmap(map, maplen);
     return;
   }
-  info_hash = accesslist_new->list;
-
-  /* No use to scan if there's not enough room for another full info_hash */
-  map_end   = map + maplen - 40;
-  read_offs = map;
-
-  /* We do ignore anything that is not of the form "^[:xdigit:]{40}[^:xdigit:].*" */
-  while (read_offs <= map_end) {
-    if (hex_tohash(*info_hash, read_offs)) {
-      read_offs += 40;
-
-      /* Append accesslist to accesslist vector */
-      if (read_offs == map_end || !hex_isdigit((unsigned char)*read_offs))
-        ++info_hash;
-    }
 
-    /* Find start of next line */
-    while (read_offs <= map_end && *(read_offs++) != '\n')
-      ;
-  }
+  found = accesslist_parse_hashes(accesslist_new->list, capacity, map, maplen);
 #ifdef _DEBUG
-  fprintf(stderr, "Added %zd info_hashes to accesslist\n", (size_t)(info_hash - accesslist_new->list));
+  fprintf(stderr, "Added %zd info_hashes to accesslist\n", found);
 #endif
 
   mmap_unmap(map, maplen);
 
-  qsort(accesslist_new->list, info_hash - accesslist_new->list, sizeof(*info_hash), vector_compare_hash);
-  accesslist_new->size = info_hash - accesslist_new->list;
+  qsort(accesslist_new->list, found, sizeof(ot_hash), vector_compare_hash);
+  accesslist_new->size = found;
 
   /* Now exchange the accesslist vector in the least race condition prone way */
   pthread_mutex_lock(&g_accesslist_mutex);
diff --git a/ot_accesslist.h b/ot_accesslist.h
index d05e1f7..b3e979d 100644
--- a/ot_accesslist.h
+++ b/ot_accesslist.h
@@ -19,6 +19,10 @@ void         accesslist_deinit(void);
 int          accesslist_hashisvalid(ot_hash hash);
 void         accesslist_cleanup(void);
 
+/* Scan buf for up to capacity info_hashes in hexadecimal notation, one per
+   line, and return how many were written to dest */
+size_t       accesslist_parse_hashes(ot_hash *dest, size_t capacity, const char *buf, size_t len);
+
 extern char *g_accesslist_filename;
 #ifdef WANT_DYNAMIC_ACCESSLIST
 extern char *g_accesslist_pipe_add;
diff --git a/tests/runner.c b/tests/runner.c
index c9385f0..bf4fbde 100644
--- a/tests/runner.c
+++ b/tests/runner.c
@@ -19,6 +19,7 @@ static const struct module modules[] = {{"scan_urlencoded_query", test_scan_urle
                                         {"ot_vector", test_vector},
                                         {"ot_random", test_random},
                                         {"ot_hex", test_hex},
+                                        {"ot_accesslist", test_accesslist},
                                         {NULL, NULL}};
 
 /* Run every module, or only those whose name contains argv[1] */
diff --git a/tests/stubs.c b/tests/stubs.c
index ae40306..febdd7b 100644
--- a/tests/stubs.c
+++ b/tests/stubs.c
@@ -12,3 +12,23 @@
 
 /* ot_vector.c calls this from vector_clean_list, which no test drives yet. */
 void free_peerlist(ot_peerlist *peer_list) { (void)peer_list; }
+
+/* ot_accesslist.c is built with WANT_ACCESSLIST_BLACK so that the hash parser
+   is reachable. These four belong to accesslist_readfile and the reload
+   worker, neither of which any test drives. */
+time_t      g_now_seconds;
+volatile int g_opentracker_running;
+
+const char *mmap_read(const char *filename, size_t *filesize);
+const char *mmap_read(const char *filename, size_t *filesize) {
+  (void)filename;
+  *filesize = 0;
+  return NULL;
+}
+
+int         mmap_unmap(const char *mapped, size_t maplen);
+int         mmap_unmap(const char *mapped, size_t maplen) {
+  (void)mapped;
+  (void)maplen;
+  return 0;
+}
diff --git a/tests/t_accesslist.c b/tests/t_accesslist.c
new file mode 100644
index 0000000..9925acd
--- /dev/null
+++ b/tests/t_accesslist.c
@@ -0,0 +1,122 @@
+/* This software was written by Dirk Engling <erdgeist@erdgeist.org>
+   It is considered beerware. Prost. Skol. Cheers or whatever.
+
+   $id$ */
+
+/* System */
+#include <stdlib.h>
+#include <string.h>
+
+/* Opentracker */
+#include "ot_accesslist.h"
+#include "test.h"
+
+#define A "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+#define B "BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB"
+#define C "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC"
+
+static ot_hash dest[8];
+
+/* The parser is handed a mapped file, so it must never look at a byte beyond
+   the length it was given. These cases place a hex digit immediately after
+   that length: a parser that reads one byte too far sees it, decides the hash
+   runs on, and silently drops the last entry. */
+
+static void case_empty(void) {
+  test_case("an empty buffer yields nothing");
+  CHECK_INT(accesslist_parse_hashes(dest, 8, "", 0), 0);
+}
+
+static void case_single_with_newline(void) {
+  const char buf[] = A "\n";
+
+  test_case("one hash followed by a newline");
+  CHECK_INT(accesslist_parse_hashes(dest, 8, buf, 41), 1);
+}
+
+static void case_single_without_newline(void) {
+  const char buf[] = A "A";
+
+  test_case("one hash ending the buffer, with a hex digit just past the end");
+  CHECK_INT(accesslist_parse_hashes(dest, 8, buf, 40), 1);
+}
+
+static void case_several_last_without_newline(void) {
+  const char buf[] = A "\n" B "\n" C "C";
+
+  test_case("a final hash without a trailing newline still counts");
+  CHECK_INT(accesslist_parse_hashes(dest, 8, buf, 3 * 40 + 2), 3);
+}
+
+static void case_run_on_hash_is_rejected(void) {
+  const char buf[] = A "A\n";
+
+  test_case("forty one hex digits are not a hash");
+  CHECK_INT(accesslist_parse_hashes(dest, 8, buf, 42), 0);
+}
+
+static void case_junk_lines_skipped(void) {
+  const char buf[] = "# a comment\n" A "\nnot a hash\n" B "\n";
+
+  test_case("lines that are not hashes are ignored");
+  CHECK_INT(accesslist_parse_hashes(dest, 8, buf, sizeof(buf) - 1), 2);
+}
+
+static void case_trailing_junk_on_line(void) {
+  const char buf[] = A " some comment\n" B "\n";
+
+  test_case("a hash followed by other text on the same line still counts");
+  CHECK_INT(accesslist_parse_hashes(dest, 8, buf, sizeof(buf) - 1), 2);
+}
+
+static void case_capacity_is_respected(void) {
+  const char buf[] = A "\n" B "\n" C "\n";
+
+  test_case("no more than capacity hashes are written");
+  CHECK_INT(accesslist_parse_hashes(dest, 2, buf, sizeof(buf) - 1), 2);
+}
+
+static void case_zero_capacity_never_writes(void) {
+  const char buf[] = A "\n";
+
+  test_case("a zero capacity tolerates a null destination");
+  CHECK_INT(accesslist_parse_hashes(NULL, 0, buf, 41), 0);
+}
+
+static void case_decoded_value(void) {
+  const char    buf[]  = "000102030405060708090A0B0C0D0E0F10111213\n";
+  const uint8_t want[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19};
+
+  test_case("the hash is decoded, not merely counted");
+  CHECK_INT(accesslist_parse_hashes(dest, 8, buf, 41), 1);
+  CHECK_MEM(dest[0], want, sizeof(want));
+}
+
+static void case_capacity_bound_holds(void) {
+  /* (len + 1) / 41 is what accesslist_readfile allocates. Pack a buffer as
+     densely as the parser allows and check the bound is not exceeded. */
+  char   buf[41 * 4];
+  size_t len, capacity, found;
+
+  test_case("the capacity formula bounds what the parser can produce");
+  memcpy(buf, A "\n" B "\n" C "\n" A, 4 * 40 + 3);
+  len      = 4 * 40 + 3;
+  capacity = (len + 1) / 41;
+  found    = accesslist_parse_hashes(dest, 8, buf, len);
+  CHECK_INT(found, 4);
+  CHECK(found <= capacity);
+}
+
+void test_accesslist(void) {
+  case_empty();
+  case_single_with_newline();
+  case_single_without_newline();
+  case_several_last_without_newline();
+  case_run_on_hash_is_rejected();
+  case_junk_lines_skipped();
+  case_trailing_junk_on_line();
+  case_capacity_is_respected();
+  case_zero_capacity_never_writes();
+  case_decoded_value();
+  case_capacity_bound_holds();
+}
diff --git a/tests/test.h b/tests/test.h
index 6af0083..8be624b 100644
--- a/tests/test.h
+++ b/tests/test.h
@@ -81,5 +81,6 @@ void test_iovec(void);
 void test_vector(void);
 void test_random(void);
 void test_hex(void);
+void test_accesslist(void);
 
 #endif