chardev/char-io.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
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
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
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 :|
© 2016 - 2024 Red Hat, Inc.