* [PATCH v2] chardev: avoid use-after-free when client disconnect
@ 2022-07-20 7:10 Hogan Wang via
2022-07-20 7:36 ` Marc-André Lureau
0 siblings, 1 reply; 6+ messages in thread
From: Hogan Wang via @ 2022-07-20 7:10 UTC (permalink / raw)
To: marcandre.lureau, berrange, qemu-devel
Cc: pbonzini, armbru, wangxinxin.wang, hogan.wang
IOWatchPoll object did not hold the @ioc and @src objects reference,
then io_watch_poll_prepare execute in IO thread, if IOWatchPoll
removed by mian 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
like this:
(gdb) bt
0 0x00007f4d121c8edf in g_source_remove_child_source (source=0x7f4c58003560, child_source=0x7f4c58009b10)
1 0x00007f4d11e0705c in io_watch_poll_prepare (source=0x7f4c58003560, timeout=timeout@entry=0x7f4c7fffed94
2 0x00007f4d121ca419 in g_main_context_prepare (context=context@entry=0x55a1463f8260, priority=priority@entry=0x7f4c7fffee20)
3 0x00007f4d121cadeb in g_main_context_iterate (context=0x55a1463f8260, block=block@entry=1, dispatch=dispatch@entry=1, self=self@entry=0x7f4c94002260)
4 0x00007f4d121cb21d in g_main_loop_run (loop=0x55a146c90920)
5 0x00007f4d11de3ea1 in iothread_run (opaque=0x55a146411820)
6 0x00007f4d11d77470 in qemu_thread_start (args=0x55a146b1f3c0)
7 0x00007f4d11f2ef3b in ?? () from /usr/lib64/libpthread.so.0
8 0x00007f4d120ba550 in clone () from /usr/lib64/libc.so.6
(gdb) p iwp
$1 = (IOWatchPoll *) 0x7f4c58003560
(gdb) p *iwp
$2 = {parent = {callback_data = 0x0,
callback_funcs = 0x0,
source_funcs = 0x7f4d11f10760 <io_watch_poll_funcs>,
ref_count = 1,
context = 0x55a1463f8260,
priority = 0,
flags = 0,
source_id = 544,
poll_fds = 0x0,
prev = 0x55a147a47a30,
next = 0x55a14712fb80,
name = 0x7f4c580036d0 "chardev-iowatch-charmonitor",
priv = 0x7f4c58003060},
ioc = 0x7f4c580033f0,
src = 0x7f4c58009b10,
fd_can_read = 0x7f4d11e0a5d0 <tcp_chr_read_poll>,
fd_read = 0x7f4d11e0a380 <tcp_chr_read>,
opaque = 0x55a1463aeea0 }
(gdb) p iwp->ioc
$3 = (QIOChannel *) 0x7f4c580033f0
(gdb) p *iwp->ioc
$4 = {parent = {class = 0x55a14707f400,
free = 0x55a146313010,
properties = 0x55a147b37b60,
ref = 0,
parent = 0x0},
features = 3,
name = 0x7f4c580085f0 "\240F",
ctx = 0x0,
read_coroutine = 0x0,
write_coroutine = 0x0}
Solution: IOWatchPoll object hold the @ioc and @src objects reference
and release the reference in GSource finalize callback function.
Signed-off-by: Hogan Wang <hogan.wang@huawei.com>
---
chardev/char-io.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/chardev/char-io.c b/chardev/char-io.c
index 4451128cba..6b10217a70 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -55,9 +55,9 @@ static gboolean io_watch_poll_prepare(GSource *source,
iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
g_source_add_child_source(source, iwp->src);
- g_source_unref(iwp->src);
} else {
g_source_remove_child_source(source, iwp->src);
+ g_source_unref(iwp->src);
iwp->src = NULL;
}
return FALSE;
@@ -69,9 +69,17 @@ static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
return G_SOURCE_CONTINUE;
}
+static void io_watch_poll_finalize(GSource *source)
+{
+ IOWatchPoll *iwp = io_watch_poll_from_source(source);
+ g_clear_pointer(&iwp->src, g_source_unref);
+ g_clear_pointer(&iwp->ioc, object_unref);
+}
+
static GSourceFuncs io_watch_poll_funcs = {
.prepare = io_watch_poll_prepare,
.dispatch = io_watch_poll_dispatch,
+ .finalize = io_watch_poll_finalize,
};
GSource *io_add_watch_poll(Chardev *chr,
@@ -88,7 +96,7 @@ GSource *io_add_watch_poll(Chardev *chr,
sizeof(IOWatchPoll));
iwp->fd_can_read = fd_can_read;
iwp->opaque = user_data;
- iwp->ioc = ioc;
+ iwp->ioc = object_ref(ioc);
iwp->fd_read = (GSourceFunc) fd_read;
iwp->src = NULL;
--
2.33.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] chardev: avoid use-after-free when client disconnect
2022-07-20 7:10 [PATCH v2] chardev: avoid use-after-free when client disconnect Hogan Wang via
@ 2022-07-20 7:36 ` Marc-André Lureau
2022-07-20 8:19 ` Daniel P. Berrangé
0 siblings, 1 reply; 6+ messages in thread
From: Marc-André Lureau @ 2022-07-20 7:36 UTC (permalink / raw)
To: Hogan Wang, Daniel P. Berrange, Markus Armbruster
Cc: QEMU, Paolo Bonzini, wangxinxin.wang
[-- Attachment #1: Type: text/plain, Size: 5127 bytes --]
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 mian thread, then io_watch_poll_prepare access @ioc or
>
mian->main
> @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?
> like this:
>
> (gdb) bt
> 0 0x00007f4d121c8edf in g_source_remove_child_source
> (source=0x7f4c58003560, child_source=0x7f4c58009b10)
> 1 0x00007f4d11e0705c in io_watch_poll_prepare (source=0x7f4c58003560,
> timeout=timeout@entry=0x7f4c7fffed94
> 2 0x00007f4d121ca419 in g_main_context_prepare (context=context@entry=0x55a1463f8260,
> priority=priority@entry=0x7f4c7fffee20)
> 3 0x00007f4d121cadeb in g_main_context_iterate (context=0x55a1463f8260,
> block=block@entry=1, dispatch=dispatch@entry=1, self=self@entry
> =0x7f4c94002260)
> 4 0x00007f4d121cb21d in g_main_loop_run (loop=0x55a146c90920)
> 5 0x00007f4d11de3ea1 in iothread_run (opaque=0x55a146411820)
> 6 0x00007f4d11d77470 in qemu_thread_start (args=0x55a146b1f3c0)
> 7 0x00007f4d11f2ef3b in ?? () from /usr/lib64/libpthread.so.0
> 8 0x00007f4d120ba550 in clone () from /usr/lib64/libc.so.6
> (gdb) p iwp
> $1 = (IOWatchPoll *) 0x7f4c58003560
> (gdb) p *iwp
> $2 = {parent = {callback_data = 0x0,
> callback_funcs = 0x0,
> source_funcs = 0x7f4d11f10760 <io_watch_poll_funcs>,
> ref_count = 1,
> context = 0x55a1463f8260,
> priority = 0,
> flags = 0,
> source_id = 544,
> poll_fds = 0x0,
> prev = 0x55a147a47a30,
> next = 0x55a14712fb80,
> name = 0x7f4c580036d0 "chardev-iowatch-charmonitor",
> priv = 0x7f4c58003060},
> ioc = 0x7f4c580033f0,
> src = 0x7f4c58009b10,
> fd_can_read = 0x7f4d11e0a5d0 <tcp_chr_read_poll>,
> fd_read = 0x7f4d11e0a380 <tcp_chr_read>,
> opaque = 0x55a1463aeea0 }
> (gdb) p iwp->ioc
> $3 = (QIOChannel *) 0x7f4c580033f0
> (gdb) p *iwp->ioc
> $4 = {parent = {class = 0x55a14707f400,
> free = 0x55a146313010,
> properties = 0x55a147b37b60,
> ref = 0,
> parent = 0x0},
> features = 3,
> name = 0x7f4c580085f0 "\240F",
> ctx = 0x0,
> read_coroutine = 0x0,
> write_coroutine = 0x0}
>
That backtrace isn't so useful. If you can produce an ASAN error though,
that would be more explicit. Not a blocker.
>
> Solution: IOWatchPoll object hold the @ioc and @src objects reference
> and release the reference in GSource finalize callback function.
>
ok, if we are not misusing GSource, otherwise seems needless or misleading
>
> Signed-off-by: Hogan Wang <hogan.wang@huawei.com>
> ---
> chardev/char-io.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-io.c b/chardev/char-io.c
> index 4451128cba..6b10217a70 100644
> --- a/chardev/char-io.c
> +++ b/chardev/char-io.c
> @@ -55,9 +55,9 @@ static gboolean io_watch_poll_prepare(GSource *source,
> iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
> g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
> g_source_add_child_source(source, iwp->src);
> - g_source_unref(iwp->src);
> } else {
> g_source_remove_child_source(source, iwp->src);
> + g_source_unref(iwp->src);
> iwp->src = NULL;
> }
> return FALSE;
> @@ -69,9 +69,17 @@ static gboolean io_watch_poll_dispatch(GSource *source,
> GSourceFunc callback,
> return G_SOURCE_CONTINUE;
> }
>
> +static void io_watch_poll_finalize(GSource *source)
> +{
> + IOWatchPoll *iwp = io_watch_poll_from_source(source);
> + g_clear_pointer(&iwp->src, g_source_unref);
> + g_clear_pointer(&iwp->ioc, object_unref);
> +}
> +
> static GSourceFuncs io_watch_poll_funcs = {
> .prepare = io_watch_poll_prepare,
> .dispatch = io_watch_poll_dispatch,
> + .finalize = io_watch_poll_finalize,
> };
>
> GSource *io_add_watch_poll(Chardev *chr,
> @@ -88,7 +96,7 @@ GSource *io_add_watch_poll(Chardev *chr,
> sizeof(IOWatchPoll));
> iwp->fd_can_read = fd_can_read;
> iwp->opaque = user_data;
> - iwp->ioc = ioc;
> + iwp->ioc = object_ref(ioc);
> iwp->fd_read = (GSourceFunc) fd_read;
> iwp->src = NULL;
>
Daniel, Markus, please take a look
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 6816 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] chardev: avoid use-after-free when client disconnect
2022-07-20 7:36 ` Marc-André Lureau
@ 2022-07-20 8:19 ` Daniel P. Berrangé
0 siblings, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2022-07-20 8:19 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Hogan Wang, Markus Armbruster, QEMU, Paolo Bonzini,
wangxinxin.wang
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 mian thread, then io_watch_poll_prepare access @ioc or
> >
>
> mian->main
>
>
> > @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
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 :|
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] chardev: avoid use-after-free when client disconnect
@ 2022-07-20 8:55 Wangjing(Hogan) via
2022-07-20 9:00 ` Daniel P. Berrangé
0 siblings, 1 reply; 6+ messages in thread
From: Wangjing(Hogan) via @ 2022-07-20 8:55 UTC (permalink / raw)
To: Daniel P. Berrangé, Marc-André Lureau
Cc: Markus Armbruster, QEMU, Paolo Bonzini, Wangxin (Alexander)
> 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 mian thread, then io_watch_poll_prepare access @ioc or
> > >
> >
> > mian->main
> >
> >
> > > @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.
>
> 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 :|
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] chardev: avoid use-after-free when client disconnect
2022-07-20 8:55 Wangjing(Hogan) via
@ 2022-07-20 9:00 ` Daniel P. Berrangé
0 siblings, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2022-07-20 9:00 UTC (permalink / raw)
To: Wangjing(Hogan)
Cc: Marc-André Lureau, Markus Armbruster, QEMU, Paolo Bonzini,
Wangxin (Alexander)
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 mian thread, then io_watch_poll_prepare access @ioc or
> > > >
> > >
> > > mian->main
> > >
> > >
> > > > @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
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 :|
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] chardev: avoid use-after-free when client disconnect
@ 2022-07-20 10:07 Wangjing(Hogan) via
0 siblings, 0 replies; 6+ messages in thread
From: Wangjing(Hogan) via @ 2022-07-20 10:07 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Marc-André Lureau, Markus Armbruster, QEMU, Paolo Bonzini,
Wangxin (Alexander)
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-20 10:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-20 7:10 [PATCH v2] chardev: avoid use-after-free when client disconnect Hogan Wang via
2022-07-20 7:36 ` Marc-André Lureau
2022-07-20 8:19 ` Daniel P. Berrangé
-- strict thread matches above, loose matches on Subject: below --
2022-07-20 8:55 Wangjing(Hogan) via
2022-07-20 9:00 ` Daniel P. Berrangé
2022-07-20 10:07 Wangjing(Hogan) via
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).