From 05efe6bde3a884d4fcca88a227efaeee2c7e3b8a Mon Sep 17 00:00:00 2001 From: Dirk Engling Date: Mon, 26 Oct 2026 23:25:21 +0100 Subject: Avoid an UB of adding NULL + 0 by keeping an offset instead of advancing the pointer into a vector struct --- ot_vector.c | 47 +++++++++++++++++++++++++++-------------------- tests/t_vector.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 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(); -- cgit v1.3