Avoid an UB of adding NULL + 0 by keeping an offset instead of advancing the pointer into a vector struct

2 files changed, 73 insertions, 20 deletions

diff --git a/ot_vector.c b/ot_vector.c
index 924d468..a56d55b 100644
--- a/ot_vector.c
+++ b/ot_vector.c
@@ -57,34 +57,39 @@ static uint8_t vector_hash_peer(ot_peer const *peer, size_t compare_size, size_t
 */
 void *vector_find_or_insert(ot_vector *vector, void *key, size_t member_size, size_t compare_size, int *exactmatch) {
   uint8_t *match = vector_binary_search(key, vector->data, vector->size, member_size, compare_size, exactmatch);
+  size_t   match_offset, bytes_after;
 
   if (*exactmatch)
     return match;
 
+  /* Hold the insertion point as an offset rather than a pointer: it survives
+     the realloc below, and an empty vector has no data pointer to offset from
+     in the first place */
+  match_offset = vector->size ? (size_t)(match - (uint8_t *)vector->data) : 0;
+  bytes_after  = member_size * vector->size - match_offset;
+
   if (vector->size + 1 > vector->space) {
-    size_t    new_space = vector->space ? OT_VECTOR_GROW_RATIO * vector->space : OT_VECTOR_MIN_MEMBERS;
-    ptrdiff_t match_off = match - (uint8_t *)vector->data;
-    uint8_t  *new_data  = realloc(vector->data, new_space * member_size);
+    size_t   new_space = vector->space ? OT_VECTOR_GROW_RATIO * vector->space : OT_VECTOR_MIN_MEMBERS;
+    uint8_t *new_data  = realloc(vector->data, new_space * member_size);
 
     if (!new_data)
       return NULL;
 
-    /* Adjust pointer if it moved by realloc */
-    match         = new_data + match_off;
-
     vector->data  = new_data;
     vector->space = new_space;
   }
-  memmove(match + member_size, match, ((uint8_t *)vector->data) + member_size * vector->size - match);
+
+  match = (uint8_t *)vector->data + match_offset;
+  memmove(match + member_size, match, bytes_after);
 
   vector->size++;
   return match;
 }
 
 ot_peer *vector_find_or_insert_peer(ot_vector *vector, ot_peer const *peer, size_t peer_size, int *exactmatch) {
-  ot_peer     *match, *end;
+  ot_peer     *match;
   const size_t compare_size = OT_PEER_COMPARE_SIZE_FROM_PEER_SIZE(peer_size);
-  size_t       match_to_end;
+  size_t       match_offset, bytes_after;
 
   /* If space is zero but size is set, we're dealing with a list of vector->size buckets */
   if (vector->space < vector->size)
@@ -94,26 +99,26 @@ ot_peer *vector_find_or_insert_peer(ot_vector *vector, ot_peer const *peer, size
   if (*exactmatch)
     return match;
 
-  /* This is the amount of bytes that needs to be pushed backwards by peer_size bytes to make room for new peer */
-  end          = (ot_peer *)vector->data + vector->size * peer_size;
-  match_to_end = end - match;
+  /* Hold the insertion point as an offset rather than a pointer: it survives
+     the realloc below, and an empty vector has no data pointer to offset from
+     in the first place. bytes_after is what has to move up by peer_size to
+     make room for the new peer */
+  match_offset = vector->size ? (size_t)(match - (ot_peer *)vector->data) : 0;
+  bytes_after  = vector->size * peer_size - match_offset;
 
   if (vector->size + 1 > vector->space) {
-    ptrdiff_t offset    = match - (ot_peer *)vector->data;
-    size_t    new_space = vector->space ? OT_VECTOR_GROW_RATIO * vector->space : OT_VECTOR_MIN_MEMBERS;
-    ot_peer  *new_data  = realloc(vector->data, new_space * peer_size);
+    size_t   new_space = vector->space ? OT_VECTOR_GROW_RATIO * vector->space : OT_VECTOR_MIN_MEMBERS;
+    ot_peer *new_data  = realloc(vector->data, new_space * peer_size);
 
     if (!new_data)
       return NULL;
-    /* Adjust pointer if it moved by realloc */
-    match         = new_data + offset;
 
     vector->data  = new_data;
     vector->space = new_space;
   }
 
-  /* Here we're guaranteed to have enough space in vector to move the block of peers after insertion point */
-  memmove(match + peer_size, match, match_to_end);
+  match = (ot_peer *)vector->data + match_offset;
+  memmove(match + peer_size, match, bytes_after);
 
   vector->size++;
   return match;
@@ -150,11 +155,13 @@ int vector_remove_peer(ot_vector *vector, ot_peer const *peer, size_t peer_size)
 }
 
 void vector_remove_torrent(ot_vector *vector, ot_torrent *match) {
-  ot_torrent *end = ((ot_torrent *)vector->data) + vector->size;
+  ot_torrent *end;
 
   if (!vector->size)
     return;
 
+  end = ((ot_torrent *)vector->data) + vector->size;
+
   free_peerlist(match->peer_list6);
   free_peerlist(match->peer_list4);
 
diff --git a/tests/t_vector.c b/tests/t_vector.c
index 541a6da..6e048e1 100644
--- a/tests/t_vector.c
+++ b/tests/t_vector.c
@@ -112,6 +112,51 @@ static void case_insert_keeps_order(void) {
   free(vector.data);
 }
 
+/* The peer variant is reached with an empty vector on every first announce
+   for a torrent, and again whenever a swarm empties and refills, because
+   vector_fixup_peers drops the allocation and nulls the data pointer. */
+
+static void case_insert_peer_into_empty_vector(void) {
+  ot_vector vector = {NULL, 0, 0};
+  ot_peer6  low, high;
+  int       exactmatch;
+  ot_peer  *slot;
+
+  memset(low, 0x11, sizeof(low));
+  memset(high, 0x99, sizeof(high));
+
+  test_case("the first peer goes into a vector that has no allocation yet");
+  slot = vector_find_or_insert_peer(&vector, low, OT_PEER_SIZE6, &exactmatch);
+  CHECK(slot != NULL);
+  CHECK_INT(exactmatch, 0);
+  memcpy(slot, low, OT_PEER_SIZE6);
+  CHECK_INT(vector.size, 1);
+  CHECK_INT(vector.space, OT_VECTOR_MIN_MEMBERS);
+  CHECK_PTR(slot, vector.data);
+
+  test_case("a second peer keeps the order");
+  slot = vector_find_or_insert_peer(&vector, high, OT_PEER_SIZE6, &exactmatch);
+  CHECK(slot != NULL);
+  CHECK_INT(exactmatch, 0);
+  memcpy(slot, high, OT_PEER_SIZE6);
+  CHECK_INT(vector.size, 2);
+  CHECK_MEM((char *)vector.data, low, OT_PEER_COMPARE_SIZE6);
+  CHECK_MEM((char *)vector.data + OT_PEER_SIZE6, high, OT_PEER_COMPARE_SIZE6);
+
+  test_case("and emptying the vector hands the allocation back");
+  CHECK(vector_remove_peer(&vector, low, OT_PEER_SIZE6) != 0);
+  CHECK(vector_remove_peer(&vector, high, OT_PEER_SIZE6) != 0);
+  CHECK_INT(vector.size, 0);
+  CHECK_PTR(vector.data, NULL);
+
+  test_case("so the next peer starts from no allocation again");
+  slot = vector_find_or_insert_peer(&vector, low, OT_PEER_SIZE6, &exactmatch);
+  CHECK(slot != NULL);
+  CHECK_INT(vector.size, 1);
+
+  free(vector.data);
+}
+
 static void case_insert_finds_existing(void) {
   ot_vector           vector = {NULL, 0, 0};
   const unsigned char five[4] = {0, 0, 0, 5};
@@ -257,6 +302,7 @@ void test_vector(void) {
   case_search_past_the_end();
   case_search_empty();
   case_insert_keeps_order();
+  case_insert_peer_into_empty_vector();
   case_insert_finds_existing();
   case_insert_compare_prefix();
   case_redistribute_holds_every_peer();