Re: [PATCH v2] chardev: avoid use-after-free when client disconnect

[Wangjing(Hogan)]

> On Wed, Jul 20, 2022 at 08:55:46AM +0000, Wangjing(Hogan) wrote:
> > 
> > > On Wed, Jul 20, 2022 at 11:36:07AM +0400, Marc-André Lureau wrote:
> > > > Hi
> > > > 
> > > > On Wed, Jul 20, 2022 at 11:13 AM Hogan Wang via 
> > > > <qemu-devel@nongnu.org>
> > > > wrote:
> > > > 
> > > > > IOWatchPoll object did not hold the @ioc and @src objects 
> > > > > reference, then io_watch_poll_prepare execute in IO thread, if 
> > > > > IOWatchPoll removed by main thread, then io_watch_poll_prepare 
> > > > > access @ioc or @src concurrently lead to coredump.
> > > > >
> > > > > In IO thread monitor scene, the IO thread used to accept client, 
> > > > > receive qmp request and handle hung-up event. Main thread used 
> > > > > to handle qmp request and send response, it will remove 
> > > > > IOWatchPoll and free @ioc when send response fail, then cause 
> > > > > use-after-free
> > > > >
> > > > 
> > > > I wonder if we are misusing GSources in that case, by removing 
> > > > sources from different threads.. Could you be more specific about 
> > > > the code path that leads to that?
> > > 
> > > It is permitted, but unfortunately every version of glib prior to 2.64 
> > > has a race condition that means you'll periodically get a use-after-free 
> > > and a crash:
> > > 
> > >   https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358
> > > 
> > > Libvirt worked around this problem by not calling 'g_source_unref'
> > > directly, but instead have a helper that uses g_idle_add to delay the 
> > > unref such that its guaranteed to happen inside the main event loop 
> > > thread.
> > > 
> > > So I'd like to know what version of glib Hogan is using
> > 
> > I am using glib2-2.62.5 in test environment, so it's looks like a glib2 
> > known issue.
> 
> Hmm, actually the fix should have been backported into the 2.62.5 release 
> according to this
> 
>   https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1361

According to the current source code:
Client connect to the monitor server, chr->gsource(IOWatchPoll) will be 
initialized by io_add_watch_poll.

According to io_watch_poll_prepare(Execute in IO Thread) function:
IOWatchPoll->src will be added to chr->gsource as a child source while the 
channel readable, but IOWatchPoll not hold the child source's reference because 
of g_source_unref(iwp->src), so left the only one reference hold by the child 
source list.

If client disconnect,then main thread will destroy the IOWatchPoll and unref 
all the child source by g_source_destroy.
Call trace:
tcp_chr_read
    tcp_chr_disconnect
        tcp_chr_disconnect_locked
            tcp_chr_free_connection
                remove_fd_in_watch
                    g_source_destroy(chr->gsource) /* unref all the child 
source */
                object_unref(OBJECT(s->ioc));      /* QIOChannel freed and 
IOWatchPoll->ioc is a wild pointer */
                s->ioc = NULL;

At this time, IOWatchPoll->src and IOWatchPoll->ioc freed, 
io_watch_poll_prepare called frequently and low-probability use-after-free.