Re: [PATCH] qio: fix qemu crash when live migration

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Aug 08, 2024 at 11:04:11AM +0800, yaozhenguo wrote:
> qemu will crash in live migration cleanup process at source host.
> BT is as below:
> 
> 0  0x00007f740fc9e165 in g_source_destroy () at /usr/lib64/libglib-2.0.so.0
> 1  0x000055a2982a0f6e in qio_net_listener_set_client_func_full
> 2  0x000055a298345130 in tcp_chr_update_read_handler
> 3  0x000055a298341598 in qemu_chr_fe_set_handlers_full
> 4  0x000055a298341655 in qemu_chr_fe_set_handlers
> 5  0x000055a298191e75 in vhost_user_blk_event
> 6  0x000055a298292b79 in object_deinit
> 7  object_finalize
> 8  object_unref
> 9  0x000055a298292b3c in object_property_del_all
> 10 object_finalize
> 11 object_unref
> 12 0x000055a298291d7d in object_property_del_child
> 13 object_unparent
> 14 0x000055a29834a3c4 in qemu_chr_cleanup
> 15 0x000055a298160d87 in qemu_cleanup
> 16 0x000055a297e6bff1 in main
> 
> Crash reason is that qio_net_listener_finalize is called before
> qio_net_listener_set_client_func_full. so, listener->io_source
> is used after free. fix this by adding more checks.

If finalize() has been called, then not only has listener->io_source
been freed, but 'listener' itself has also been freed, thus....

> 
> Signed-off-by: yaozhenguo <yaozhenguo@jd.com>
> ---
>  io/net-listener.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/io/net-listener.c b/io/net-listener.c
> index 47405965a6..c02965f919 100644
> --- a/io/net-listener.c
> +++ b/io/net-listener.c
> @@ -143,6 +143,11 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
>  {
>      size_t i;
>  
> +
> +    if (!listener->nsioc || !listener->io_source || !listener->name) {
> +        return;
> +    }

....this is still accessing freed memory for 'listener'.


What is the call path of the stack triggering qio_net_listener_finalize ?

Whatever callpath has done that needs to be setting SocketChardev->listener
field to NULL, because tcp_chr_update_read_handler will check for NULL
before calling qio_net_listener_set_client_func_full.

> +
>      if (listener->io_notify) {
>          listener->io_notify(listener->io_data);
>      }
> @@ -264,6 +269,10 @@ void qio_net_listener_disconnect(QIONetListener *listener)
>  {
>      size_t i;
>  
> +    if (!listener->nsioc || !listener->io_source || !listener->name) {
> +        return;
> +    }
> +
>      if (!listener->connected) {
>          return;
>      }
> @@ -301,6 +310,10 @@ static void qio_net_listener_finalize(Object *obj)
>      g_free(listener->io_source);
>      g_free(listener->sioc);
>      g_free(listener->name);
> +
> +    listener->io_source = NULL;
> +    listener->sioc = NULL;
> +    listener->name = NULL;
>  }
>  
>  static const TypeInfo qio_net_listener_info = {
> -- 
> 2.43.0
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|