All new functionality, cleanup and similar things to the hash implementation were done in alfred. So import the newest version to keep it in sync.
Signed-off-by: Sven Eckelmann sven@narfation.org --- hash.c | 88 +++++++++++++++++++++++++++++++++++++++++------------------------- hash.h | 69 ++++++++++++++++++++++++++++++--------------------- 2 files changed, 96 insertions(+), 61 deletions(-)
diff --git a/hash.c b/hash.c index 2a9f557..7271b89 100644 --- a/hash.c +++ b/hash.c @@ -20,11 +20,11 @@ */
-#include <stdio.h> /* NULL */ #include "hash.h" +#include <stdlib.h> +#include <stdio.h> #include "allocate.h"
- /* clears the hash */ void hash_init(struct hashtable_t *hash) { @@ -70,9 +70,17 @@ void hash_destroy(struct hashtable_t *hash) debugFree(hash, 1303); }
+/* free hash_it_t pointer when stopping hash_iterate early */ +void hash_iterate_free(struct hash_it_t *iter_in) +{ + debugFree(iter_in, 1304); +} + /* iterate though the hash. first element is selected with iter_in NULL. - * use the returned iterator to access the elements until hash_it_t returns NULL. */ -struct hash_it_t *hash_iterate(struct hashtable_t *hash, struct hash_it_t *iter_in) + * use the returned iterator to access the elements until hash_it_t returns + * NULL. */ +struct hash_it_t *hash_iterate(struct hashtable_t *hash, + struct hash_it_t *iter_in) { struct hash_it_t *iter;
@@ -81,14 +89,17 @@ struct hash_it_t *hash_iterate(struct hashtable_t *hash, struct hash_it_t *iter_ iter->index = -1; iter->bucket = NULL; iter->prev_bucket = NULL; - } else - iter= iter_in; + } else { + iter = iter_in; + }
- /* sanity checks first (if our bucket got deleted in the last iteration): */ + /* sanity checks first (if our bucket got deleted in the last + * iteration): */ if (iter->bucket != NULL) { if (iter->first_bucket != NULL) {
- /* we're on the first element and it got removed after the last iteration. */ + /* we're on the first element and it got removed after + * the last iteration. */ if ((*iter->first_bucket) != iter->bucket) {
/* there are still other elements in the list */ @@ -105,9 +116,10 @@ struct hash_it_t *hash_iterate(struct hashtable_t *hash, struct hash_it_t *iter_
} else if (iter->prev_bucket != NULL) {
- /* we're not on the first element, and the bucket got removed after the last iteration. - * the last bucket's next pointer is not pointing to our actual bucket anymore. - * select the next. */ + /* we're not on the first element, and the bucket got + * removed after the last iteration. The last bucket's + * next pointer is not pointing to our actual bucket + * anymore. Select the next. */ if (iter->prev_bucket->next != iter->bucket) iter->bucket = iter->prev_bucket;
@@ -124,7 +136,8 @@ struct hash_it_t *hash_iterate(struct hashtable_t *hash, struct hash_it_t *iter_ return iter; } } - /* if not returned yet, we've reached the last one on the index and have to search forward */ + /* if not returned yet, we've reached the last one on the index and + * have to search forward */
iter->index++; /* go through the entries of the hash table */ @@ -142,12 +155,13 @@ struct hash_it_t *hash_iterate(struct hashtable_t *hash, struct hash_it_t *iter_ }
/* nothing to iterate over anymore */ - debugFree(iter, 1304); + hash_iterate_free(iter); return NULL; }
/* allocates and clears the hash */ -struct hashtable_t *hash_new(int size, hashdata_compare_cb compare, hashdata_choose_cb choose) +struct hashtable_t *hash_new(int size, hashdata_compare_cb compare, + hashdata_choose_cb choose) { struct hashtable_t *hash;
@@ -155,8 +169,8 @@ struct hashtable_t *hash_new(int size, hashdata_compare_cb compare, hashdata_cho if (!hash) return NULL;
- hash->size= size; - hash->table= debugMalloc(sizeof(struct element_t *) * size, 303); + hash->size = size; + hash->table = debugMalloc(sizeof(struct element_t *)*size, 303);
if (!hash->table) { debugFree(hash, 1305); @@ -183,11 +197,11 @@ int hash_add(struct hashtable_t *hash, void *data) return -1;
prev_bucket = bucket; - bucket= bucket->next; + bucket = bucket->next; }
/* found the tail of the list, add new element */ - bucket = debugMalloc(sizeof(struct element_t),304); + bucket = debugMalloc(sizeof(struct element_t), 304);
if (!bucket) return -1; @@ -197,17 +211,17 @@ int hash_add(struct hashtable_t *hash, void *data) bucket->next = NULL;
/* and link it */ - if (prev_bucket == NULL) { + if (prev_bucket == NULL) hash->table[index] = bucket; - } else { + else prev_bucket->next = bucket; - }
hash->elements++; return 0; }
-/* finds data, based on the key in keydata. returns the found data on success, or NULL on error */ +/* finds data, based on the key in keydata. returns the found data on success, + * or NULL on error */ void *hash_find(struct hashtable_t *hash, void *keydata) { int index; @@ -226,20 +240,21 @@ void *hash_find(struct hashtable_t *hash, void *keydata) return NULL; }
-/* remove bucket (this might be used in hash_iterate() if you already found the bucket - * you want to delete and don't need the overhead to find it again with hash_remove(). - * But usually, you don't want to use this function, as it fiddles with hash-internals. */ +/* remove bucket (this might be used in hash_iterate() if you already found + * the bucket you want to delete and don't need the overhead to find it again + * with hash_remove(). But usually, you don't want to use this function, as it + * fiddles with hash-internals. */ void *hash_remove_bucket(struct hashtable_t *hash, struct hash_it_t *hash_it_t) { void *data_save;
- data_save = hash_it_t->bucket->data; /* save the pointer to the data */ + /* save the pointer to the data */ + data_save = hash_it_t->bucket->data;
- if (hash_it_t->prev_bucket != NULL) { + if (hash_it_t->prev_bucket != NULL) hash_it_t->prev_bucket->next = hash_it_t->bucket->next; - } else if (hash_it_t->first_bucket != NULL) { + else if (hash_it_t->first_bucket != NULL) (*hash_it_t->first_bucket) = hash_it_t->bucket->next; - }
debugFree(hash_it_t->bucket, 1306);
@@ -261,7 +276,12 @@ void *hash_remove(struct hashtable_t *hash, void *data)
while (hash_it_t.bucket != NULL) { if (hash->compare(hash_it_t.bucket->data, data)) { - hash_it_t.first_bucket = (hash_it_t.bucket == hash->table[hash_it_t.index] ? &hash->table[ hash_it_t.index ] : NULL); + int bucket_same; + bucket_same = (hash_it_t.bucket == + hash->table[hash_it_t.index]); + hash_it_t.first_bucket = (bucket_same ? + &hash->table[hash_it_t.index] : + NULL); return hash_remove_bucket(hash, &hash_it_t); }
@@ -272,7 +292,8 @@ void *hash_remove(struct hashtable_t *hash, void *data) return NULL; }
-/* resize the hash, returns the pointer to the new hash or NULL on error. removes the old hash on success. */ +/* resize the hash, returns the pointer to the new hash or NULL on error. + * removes the old hash on success. */ struct hashtable_t *hash_resize(struct hashtable_t *hash, int size) { struct hashtable_t *new_hash; @@ -292,8 +313,9 @@ struct hashtable_t *hash_resize(struct hashtable_t *hash, int size) bucket = bucket->next; } } - - hash_delete(hash, NULL); /* remove hash and eventual overflow buckets but not the content itself. */ + /* remove hash and eventual overflow buckets but not the + * content itself. */ + hash_delete(hash, NULL); return new_hash; }
diff --git a/hash.h b/hash.h index 66d7162..1682eb8 100644 --- a/hash.h +++ b/hash.h @@ -28,8 +28,8 @@ typedef int (*hashdata_choose_cb)(void *, int); typedef void (*hashdata_free_cb)(void *);
struct element_t { - void *data; /* pointer to the data */ - struct element_t *next; /* overflow bucket pointer */ + void *data; /* pointer to the data */ + struct element_t *next; /* overflow bucket pointer */ };
struct hash_it_t { @@ -40,56 +40,69 @@ struct hash_it_t { };
struct hashtable_t { - struct element_t **table; /* the hashtable itself, with the buckets */ - int elements; /* number of elements registered */ - int size; /* size of hashtable */ - hashdata_compare_cb compare; /* callback to a compare function. - * should compare 2 element datas for their keys, - * return 0 if same and not 0 if not same */ - hashdata_choose_cb choose; /* the hashfunction, should return an index based - * on the key in the data of the first argument - * and the size the second */ + struct element_t **table; /* the hashtable itself, with the + * buckets */ + int elements; /* number of elements registered */ + int size; /* size of hashtable */ + hashdata_compare_cb compare; /* callback to a compare function. + * should compare 2 element datas for + * their keys, return 0 if same and not + * 0 if not same */ + hashdata_choose_cb choose; /* the hashfunction, should return an + * index based on the key in the data + * of the first argument and the size + * the second */ };
/* clears the hash */ -void hash_init(struct hashtable_t *hash); +void hash_init(struct hashtable_t *hash);
/* allocates and clears the hash */ -struct hashtable_t *hash_new(int size, hashdata_compare_cb compare, hashdata_choose_cb choose); +struct hashtable_t *hash_new(int size, hashdata_compare_cb compare, + hashdata_choose_cb choose);
-/* remove bucket (this might be used in hash_iterate() if you already found the bucket - * you want to delete and don't need the overhead to find it again with hash_remove(). - * But usually, you don't want to use this function, as it fiddles with hash-internals. */ -void *hash_remove_bucket(struct hashtable_t *hash, struct hash_it_t *hash_it_t); +/* remove bucket (this might be used in hash_iterate() if you already found + * the bucket you want to delete and don't need the overhead to find it again + * with hash_remove(). But usually, you don't want to use this function, as it + * fiddles with hash-internals. */ +void *hash_remove_bucket(struct hashtable_t *hash, + struct hash_it_t *hash_it_t);
/* remove the hash structure. if hashdata_free_cb != NULL, * this function will be called to remove the elements inside of the hash. * if you don't remove the elements, memory might be leaked. */ -void hash_delete(struct hashtable_t *hash, hashdata_free_cb free_cb); +void hash_delete(struct hashtable_t *hash, hashdata_free_cb free_cb);
/* free only the hashtable and the hash itself. */ -void hash_destroy(struct hashtable_t *hash); +void hash_destroy(struct hashtable_t *hash);
/* adds data to the hashtable. returns 0 on success, -1 on error */ -int hash_add(struct hashtable_t *hash, void *data); +int hash_add(struct hashtable_t *hash, void *data);
/* removes data from hash, if found. returns pointer do data on success, * so you can remove the used structure yourself, or NULL on error . * data could be the structure you use with just the key filled, * we just need the key for comparing. */ -void *hash_remove(struct hashtable_t *hash, void *data); +void *hash_remove(struct hashtable_t *hash, void *data);
-/* finds data, based on the key in keydata. returns the found data on success, or NULL on error */ -void *hash_find(struct hashtable_t *hash, void *keydata); +/* finds data, based on the key in keydata. returns the found data on success, + * or NULL on error */ +void *hash_find(struct hashtable_t *hash, void *keydata);
-/* resize the hash, returns the pointer to the new hash or NULL on error. removes the old hash on success */ -struct hashtable_t *hash_resize(struct hashtable_t *hash, int size); +/* resize the hash, returns the pointer to the new hash or NULL on error. + * removes the old hash on success */ +struct hashtable_t *hash_resize(struct hashtable_t *hash, int size);
/* print the hash table for debugging */ -void hash_debug( struct hashtable_t *hash); +void hash_debug(struct hashtable_t *hash);
/* iterate though the hash. first element is selected with iter_in NULL. - * use the returned iterator to access the elements until hash_it_t returns NULL. */ -struct hash_it_t *hash_iterate(struct hashtable_t *hash, struct hash_it_t *iter_in); + * use the returned iterator to access the elements until hash_it_t + * returns NULL. */ +struct hash_it_t *hash_iterate(struct hashtable_t *hash, + struct hash_it_t *iter_in); + +/* free hash_it_t pointer when stopping hash_iterate early */ +void hash_iterate_free(struct hash_it_t *iter_in);
#endif
The current implementation of hash_resize uses hash_add directly to initialize a new hash table. But hash_add has two possible situations when it returns an error and hash_resize would leak the data:
* data already exists * malloc fails
The check for the duplicated data is not really harmful (beside increasing the time to re-add elements) but the malloc can potentially return an error. This malloc is unnecessary and just takes extra time. Instead the bucket from the old hash table can be re-used.
Signed-off-by: Sven Eckelmann sven@narfation.org --- hash.c | 72 ++++++++++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 44 insertions(+), 28 deletions(-)
diff --git a/hash.c b/hash.c index 7271b89..1c551dc 100644 --- a/hash.c +++ b/hash.c @@ -63,6 +63,40 @@ void hash_delete(struct hashtable_t *hash, hashdata_free_cb free_cb) hash_destroy(hash); }
+/* adds data to the hashtable and reuse bucket. + * returns 0 on success, -1 on error */ +static int hash_add_bucket(struct hashtable_t *hash, void *data, + struct element_t *bucket, int check_duplicate) +{ + int index; + struct element_t *bucket_it, *prev_bucket = NULL; + + index = hash->choose(data, hash->size); + bucket_it = hash->table[index]; + + while (bucket_it != NULL) { + if (check_duplicate && + hash->compare(bucket_it->data, data)) + return -1; + + prev_bucket = bucket_it; + bucket_it = bucket_it->next; + } + + /* init the new bucket */ + bucket->data = data; + bucket->next = NULL; + + /* and link it */ + if (prev_bucket == NULL) + hash->table[index] = bucket; + else + prev_bucket->next = bucket; + + hash->elements++; + return 0; +} + /* free only the hashtable and the hash itself. */ void hash_destroy(struct hashtable_t *hash) { @@ -186,19 +220,8 @@ struct hashtable_t *hash_new(int size, hashdata_compare_cb compare, /* adds data to the hashtable. returns 0 on success, -1 on error */ int hash_add(struct hashtable_t *hash, void *data) { - int index; - struct element_t *bucket, *prev_bucket = NULL; - - index = hash->choose(data, hash->size); - bucket = hash->table[index]; - - while (bucket != NULL) { - if (hash->compare(bucket->data, data)) - return -1; - - prev_bucket = bucket; - bucket = bucket->next; - } + int ret; + struct element_t *bucket;
/* found the tail of the list, add new element */ bucket = debugMalloc(sizeof(struct element_t), 304); @@ -206,18 +229,11 @@ int hash_add(struct hashtable_t *hash, void *data) if (!bucket) return -1;
- /* init the new bucket */ - bucket->data = data; - bucket->next = NULL; - - /* and link it */ - if (prev_bucket == NULL) - hash->table[index] = bucket; - else - prev_bucket->next = bucket; + ret = hash_add_bucket(hash, data, bucket, 1); + if (ret < 0) + debugFree(bucket, 1307);
- hash->elements++; - return 0; + return ret; }
/* finds data, based on the key in keydata. returns the found data on success, @@ -307,10 +323,10 @@ struct hashtable_t *hash_resize(struct hashtable_t *hash, int size)
/* copy the elements */ for (i = 0; i < hash->size; i++) { - bucket = hash->table[i]; - while (bucket != NULL) { - hash_add(new_hash, bucket->data); - bucket = bucket->next; + while (hash->table[i]) { + bucket = hash->table[i]; + hash->table[i] = bucket->next; + hash_add_bucket(new_hash, bucket->data, bucket, 0); } } /* remove hash and eventual overflow buckets but not the
The default type of an integer constant is int. This reduced the possible bits for a shift to 32. But the size of uintmax_t is most likely larger (64 bit). Thus the upper 32 bit cannot be accessed correctly with this bitarray implementation.
Signed-off-by: Sven Eckelmann sven@narfation.org --- bitarray.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/bitarray.c b/bitarray.c index 133f6e7..354eddd 100644 --- a/bitarray.c +++ b/bitarray.c @@ -49,7 +49,7 @@ uint8_t get_bit_status( TYPE_OF_WORD *seq_bits, uint16_t last_seqno, uint16_t cu word_offset= ( last_seqno - curr_seqno ) % WORD_BIT_SIZE; /* which position in the selected word */ word_num = ( last_seqno - curr_seqno ) / WORD_BIT_SIZE; /* which word */
- if ( seq_bits[word_num] & 1<<word_offset ) /* get position status */ + if ( seq_bits[word_num] & (1ULL << word_offset)) /* get position status */ return 1; else return 0; @@ -72,7 +72,7 @@ void bit_mark( TYPE_OF_WORD *seq_bits, int32_t n ) { word_offset= n%WORD_BIT_SIZE; /* which position in the selected word */ word_num = n/WORD_BIT_SIZE; /* which word */
- seq_bits[word_num]|= 1<<word_offset; /* turn the position on */ + seq_bits[word_num] |= 1ULL << word_offset; /* turn the position on */ }
/* shift the packet array p by n places. */
client_to_gw_tun calls setsockopt which can fail. In this case it jumps to the error handling and cleanup code but doesn't close the udp_sock. This has to be done to avoid leaking of file descriptors.
Signed-off-by: Sven Eckelmann sven@narfation.org --- posix/tunnel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/posix/tunnel.c b/posix/tunnel.c index 08c56a2..8e7611b 100644 --- a/posix/tunnel.c +++ b/posix/tunnel.c @@ -201,7 +201,7 @@ void *client_to_gw_tun(void *arg) sock_opts = 1; if (setsockopt(udp_sock, SOL_SOCKET, SO_REUSEADDR, &sock_opts, sizeof(sock_opts)) < 0) { debug_output(0, "Error - can't set options on udp socket: %s\n", strerror(errno)); - goto out; + goto udp_out; }
if (bind(udp_sock, (struct sockaddr *)&my_addr, sizeof(struct sockaddr_in)) < 0) {
The _hna_global_add function searches through the list of HNAs to check if there is already a hna for this orig pointer. Otherwise it is automatically allocated.
This was implemented by setting hna_orig_ptr to NULL after the comparison. This will most likely crash the program because the list_for_each_entry implementation uses hna_orig_ptr to find the next entry before doing the loop condition check. The next entry cannot be found by dereferencing NULL+epsilon.
Signed-off-by: Sven Eckelmann sven@narfation.org --- hna.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hna.c b/hna.c index f0d64e1..0f28bfe 100644 --- a/hna.c +++ b/hna.c @@ -322,6 +322,7 @@ static void _hna_global_add(struct orig_node *orig_node, struct hna_element *hna struct hna_orig_ptr *hna_orig_ptr = NULL; struct orig_node *old_orig_node = NULL; struct hashtable_t *swaphash; + int found = 0;
hna_global_entry = ((struct hna_global_entry *)hash_find(hna_global_hash, hna_element));
@@ -354,14 +355,14 @@ static void _hna_global_add(struct orig_node *orig_node, struct hna_element *hna return;
list_for_each_entry(hna_orig_ptr, &hna_global_entry->orig_list, list) { - if (hna_orig_ptr->orig_node == orig_node) + if (hna_orig_ptr->orig_node == orig_node) { + found = 1; break; - - hna_orig_ptr = NULL; + } }
/* append the given orig node to the list */ - if (!hna_orig_ptr) { + if (!found) { hna_orig_ptr = debugMalloc(sizeof(struct hna_orig_ptr), 704);
if (!hna_orig_ptr)
send_outstanding_packets checks if a forw_node has a correct if_incoming. Otherwise it jumps to packet_free to deallocate the packet infrastructure. But this also schedules packets with the batman interfaces as target incoming_if. This is known to be NULL but is dereferenced in schedule_own_packet.
This NULL dereference should be avoided.
Signed-off-by: Sven Eckelmann sven@narfation.org --- schedule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/schedule.c b/schedule.c index 3e65d68..03cdb78 100644 --- a/schedule.c +++ b/schedule.c @@ -359,7 +359,7 @@ void send_outstanding_packets(uint32_t curr_time)
packet_free: list_del((struct list_head *)&forw_list, forw_pos, &forw_list);
- if (forw_node->own) + if (forw_node->own && forw_node->if_incoming) schedule_own_packet(forw_node->if_incoming);
debugFree(forw_node->pack_buff, 1501);
The update_route functions first stores the orig_node->router in an extra variable and later checks if orig_node is NULL. This is not only a potential cause of a crash but can also cause new compilers to drop the NULL check completely [1].
[1] https://gcc.gnu.org/onlinedocs/gcc-3.4.3/gcc/Optimize-Options.html#index-fde...
Signed-off-by: Sven Eckelmann sven@narfation.org --- batman.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/batman.c b/batman.c index 89b3bf1..c3dfa7f 100644 --- a/batman.c +++ b/batman.c @@ -353,10 +353,9 @@ void update_routes(struct orig_node *orig_node, struct neigh_node *neigh_node, u prof_start(PROF_update_routes); debug_output(4, "update_routes() \n");
- old_router = orig_node->router; - /* also handles orig_node->router == NULL and neigh_node == NULL */ if ((orig_node != NULL) && (orig_node->router != neigh_node)) { + old_router = orig_node->router;
if ( ( orig_node != NULL ) && ( neigh_node != NULL ) ) { addr_to_string( orig_node->orig, orig_str, ADDR_STR_LEN ); @@ -415,7 +414,8 @@ void update_routes(struct orig_node *orig_node, struct neigh_node *neigh_node, u orig_node->router = neigh_node;
} else if (orig_node != NULL) { - hna_global_update(orig_node, hna_recv_buff, hna_buff_len, old_router); + hna_global_update(orig_node, hna_recv_buff, hna_buff_len, + orig_node->router); }
prof_stop(PROF_update_routes);
Hello Sven –
I have applied your pending patches in 036ae...
Many thanks for your kind support!
Cheers, Elektra
b.a.t.m.a.n@lists.open-mesh.org