commit 8c29729916be9cd9692657096babb838fc2e7a4c Author: David Goulet dgoulet@torproject.org Date: Tue Apr 20 13:13:54 2021 -0400

hs: Fix memory leak in client cache

Fixes #40356

Signed-off-by: David Goulet dgoulet@torproject.org --- changes/ticket40356 | 3 +++ src/feature/hs/hs_cache.c | 61 ++++++++++++++++++++++++++++------------------- 2 files changed, 39 insertions(+), 25 deletions(-)

diff --git a/changes/ticket40356 b/changes/ticket40356 new file mode 100644 index 0000000000..59c32ce0cc --- /dev/null +++ b/changes/ticket40356 @@ -0,0 +1,3 @@ + o Minor bugfix (onion service, client, memory leak): + - An expired cached descriptor could have been overwritten with a new one + leading to a memory leak. Fixes bug 40356; bugfix on 0.3.5.1-alpha. diff --git a/src/feature/hs/hs_cache.c b/src/feature/hs/hs_cache.c index c1334a7d27..9c35936748 100644 --- a/src/feature/hs/hs_cache.c +++ b/src/feature/hs/hs_cache.c @@ -353,6 +353,31 @@ static digest256map_t *hs_cache_v3_client; * objects all related to a specific service. */ static digest256map_t *hs_cache_client_intro_state;

+#define cache_client_desc_free(val) \ + FREE_AND_NULL(hs_cache_client_descriptor_t, cache_client_desc_free_, (val)) + +/** Free memory allocated by <b>desc</b>. */ +static void +cache_client_desc_free_(hs_cache_client_descriptor_t *desc) +{ + if (desc == NULL) { + return; + } + hs_descriptor_free(desc->desc); + memwipe(&desc->key, 0, sizeof(desc->key)); + memwipe(desc->encoded_desc, 0, strlen(desc->encoded_desc)); + tor_free(desc->encoded_desc); + tor_free(desc); +} + +/** Helper function: Use by the free all function to clear the client cache */ +static void +cache_client_desc_free_void(void *ptr) +{ + hs_cache_client_descriptor_t *desc = ptr; + cache_client_desc_free(desc); +} + /** Return the size of a client cache entry in bytes. */ static size_t cache_get_client_entry_size(const hs_cache_client_descriptor_t *entry) @@ -390,7 +415,18 @@ remove_v3_desc_as_client(const hs_cache_client_descriptor_t *desc) static void store_v3_desc_as_client(hs_cache_client_descriptor_t *desc) { + hs_cache_client_descriptor_t *cached_desc; + tor_assert(desc); + + /* Because the lookup function doesn't return an expired entry, it can linger + * in the cache until we clean it up or a new descriptor is stored. So, + * before adding, we'll make sure we are not overwriting an old descriptor + * (which is OK in terms of semantic) but leads to memory leak. */ + cached_desc = digest256map_get(hs_cache_v3_client, desc->key.pubkey); + if (cached_desc) { + cache_client_desc_free(cached_desc); + } digest256map_set(hs_cache_v3_client, desc->key.pubkey, desc); /* Update cache size with this entry for the OOM handler. */ rend_cache_increment_allocation(cache_get_client_entry_size(desc)); @@ -473,31 +509,6 @@ cache_client_desc_new(const char *desc_str, return client_desc; }

-#define cache_client_desc_free(val) \ - FREE_AND_NULL(hs_cache_client_descriptor_t, cache_client_desc_free_, (val)) - -/** Free memory allocated by <b>desc</b>. */ -static void -cache_client_desc_free_(hs_cache_client_descriptor_t *desc) -{ - if (desc == NULL) { - return; - } - hs_descriptor_free(desc->desc); - memwipe(&desc->key, 0, sizeof(desc->key)); - memwipe(desc->encoded_desc, 0, strlen(desc->encoded_desc)); - tor_free(desc->encoded_desc); - tor_free(desc); -} - -/** Helper function: Use by the free all function to clear the client cache */ -static void -cache_client_desc_free_void(void *ptr) -{ - hs_cache_client_descriptor_t *desc = ptr; - cache_client_desc_free(desc); -} - /** Return a newly allocated and initialized hs_cache_intro_state_t object. */ static hs_cache_intro_state_t * cache_intro_state_new(void)