Hi
On Mon, Mar 28, 2022 at 12:22 PM 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, io_watch_poll_prepare may execute at last
> chance and access the freed @ioc or @src object.
>
> 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.w...@huawei.com>
> ---
> chardev/char-io.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char-io.c b/chardev/char-io.c
> index 8ced184160..3af388eeaa 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);
>
This change looks unnecessary. According to g_source_add_child_source()
documentation: *"**source* will hold a reference on *child_source* while
*child_source* is attached to it."
> iwp->src = NULL;
> }
> return FALSE;
> @@ -69,9 +69,23 @@ 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);
> + if (iwp->src) {
> + g_source_unref(iwp->src);
>
(see above)
> + iwp->src = NULL;
> + }
> + if (iwp->ioc) {
> + object_unref(OBJECT(iwp->ioc));
> + iwp->ioc = NULL;
> + }
>
replace with 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,6 +102,7 @@ GSource *io_add_watch_poll(Chardev *chr,
> sizeof(IOWatchPoll));
> iwp->fd_can_read = fd_can_read;
> iwp->opaque = user_data;
> + object_ref(OBJECT(ioc));
> iwp->ioc = ioc;
>
please use "iwp->ioc = object_ref(ioc)"
> iwp->fd_read = (GSourceFunc) fd_read;
> iwp->src = NULL;
> --
> 2.33.0
>
>
>
Daniel, please review
thanks
--
Marc-André Lureau
