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...@linux.intel.com> > Signed-off-by: Zhang Chen <chen.zh...@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); > } >