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

1 files changed, 33 insertions, 30 deletions

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);