Re: [PATCH 2/4] net/colo: Fix a "double free" crash to clear the conn_list

[address@hidden]

On 09/03/2022 16:38, Zhang Chen wrote:
> We notice the QEMU may crash when the guest has too many
> incoming network connections with the following log:
>
> 15197@1593578622.668573:colo_proxy_main : colo proxy connection hashtable 
> full, clear it
> free(): invalid pointer
> [1]    15195 abort (core dumped)  qemu-system-x86_64 ....
>
> This is because we create the s->connection_track_table with
> g_hash_table_new_full() which is defined as:
>
> GHashTable * g_hash_table_new_full (GHashFunc hash_func,
>                         GEqualFunc key_equal_func,
>                         GDestroyNotify key_destroy_func,
>                         GDestroyNotify value_destroy_func);
>
> The fourth parameter connection_destroy() will be called to free the
> memory allocated for all 'Connection' values in the hashtable when
> we call g_hash_table_remove_all() in the connection_hashtable_reset().
>
> It's unnecessary because we clear the conn_list explicitly later,
> and it's buggy when other agents try to call connection_get()
> with the same connection_track_table.
>
> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>   net/colo-compare.c    | 2 +-
>   net/filter-rewriter.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 62554b5b3c..ab054cfd21 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -1324,7 +1324,7 @@ static void colo_compare_complete(UserCreatable *uc, 
> Error **errp)
>       s->connection_track_table = g_hash_table_new_full(connection_key_hash,
>                                                         connection_key_equal,
>                                                         g_free,
> -                                                      connection_destroy);
> +                                                      NULL);


202 /* if not found, create a new connection and add to hash table */
203 Connection *connection_get(GHashTable *connection_track_table,
204                            ConnectionKey *key,
205                            GQueue *conn_list)
206 {
207     Connection *conn = g_hash_table_lookup(connection_track_table, key);
208
209     if (conn == NULL) {
210         ConnectionKey *new_key = g_memdup(key, sizeof(*key));
211
212         conn = connection_new(key);
213
214         if (g_hash_table_size(connection_track_table) > HASHTABLE_MAX_SIZE) 
{
215             trace_colo_proxy_main("colo proxy connection hashtable full,"
216                                   " clear it");
217 connection_hashtable_reset(connection_track_table);

197 void connection_hashtable_reset(GHashTable *connection_track_table)
198 {
199 g_hash_table_remove_all(connection_track_table);
200 }

IIUC,  above subroutine will do some cleanup explicitly. And before your patch, 
connection_hashtable_reset()
will release all keys and their values in this hashtable. But now, you remove 
all keys and just
one value(conn_list) instead. Does it means other values will be leaked ?


218 /*
219              * clear the conn_list
220 */
221             while (!g_queue_is_empty(conn_list)) {
222 connection_destroy(g_queue_pop_head(conn_list));
223 }
224 }
225
226         g_hash_table_insert(connection_track_table, new_key, conn);
227 }
228
229     return conn;
230 }


Thanks
Zhijian

>   
>       colo_compare_iothread(s);
>   
> diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
> index bf05023dc3..c18c4c2019 100644
> --- a/net/filter-rewriter.c
> +++ b/net/filter-rewriter.c
> @@ -383,7 +383,7 @@ static void colo_rewriter_setup(NetFilterState *nf, Error 
> **errp)
>       s->connection_track_table = g_hash_table_new_full(connection_key_hash,
>                                                         connection_key_equal,
>                                                         g_free,
> -                                                      connection_destroy);
> +                                                      NULL);
>       s->incoming_queue = qemu_new_net_queue(qemu_netfilter_pass_to_next, nf);
>   }
>