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

[Zhenguo Yao]

Hi Daniel. Sorry,  I don't notice that this is a fixed issue by
b8a7f51f59e28d5a8e0c07ed3919cc9695560ed2(chardev/char-socket: set
s->listener = NULL in char_socket_finalize).
the following process can lead this issue：

char_socket_finalize->object_unref(OBJECT(s->listener)); // free
io_source, free s->listener. but s->listener is not clear.
char_socket_finalize->qemu_chr_be_event(chr, CHR_EVENT_CLOSED)->
qio_net_listener_set_client_func_full //
g_source_destroy(listener->io_source[i]); memory fault

Daniel P. Berrangé <berrange@redhat.com> 于2024年8月8日周四 16:50写道：
>
> 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 :|
>