On Wed, Feb 17, 2021 at 2:48 PM Daniel P. Berrangé <berra...@redhat.com>
wrote:
> On Wed, Feb 17, 2021 at 12:24:03PM +0400, marcandre.lur...@redhat.com
> wrote:
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > Spotted by ASAN:
> >
> > ==2407186==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x6020003ac4f0 at pc 0x7ffff766659c bp 0x7fffffffd1d0 sp 0x7fffffffc980
> > READ of size 1 at 0x6020003ac4f0 thread T0
> > #0 0x7ffff766659b (/lib64/libasan.so.6+0x8a59b)
> > #1 0x7ffff6bfa843 in g_str_equal ../glib/ghash.c:2303
> > #2 0x7ffff6bf8167 in g_hash_table_lookup_node ../glib/ghash.c:493
> > #3 0x7ffff6bf9b78 in g_hash_table_insert_internal
> ../glib/ghash.c:1598
> > #4 0x7ffff6bf9c32 in g_hash_table_add ../glib/ghash.c:1689
> > #5 0x5555596caad4 in module_load_one ../util/module.c:233
> > #6 0x5555596ca949 in module_load_one ../util/module.c:225
> > #7 0x5555596ca949 in module_load_one ../util/module.c:225
> > #8 0x5555596cbdf4 in module_load_qom_all ../util/module.c:349
> > #9 0x5555593c6bbc in qmp_qom_list_types ../qom/qom-qmp-cmds.c:114
> > #10 0x5555595576df in qmp_marshal_qom_list_types
> qapi/qapi-commands-qom.c:194
> > #11 0x555559772868 in do_qmp_dispatch_bh ../qapi/qmp-dispatch.c:110
> > #12 0x5555596f8786 in aio_bh_call ../util/async.c:136
> > #13 0x5555596f8e9b in aio_bh_poll ../util/async.c:164
> > #14 0x555559685803 in aio_dispatch ../util/aio-posix.c:381
> > #15 0x5555596fa324 in aio_ctx_dispatch ../util/async.c:306
> > #16 0x7ffff6c0deda in g_main_dispatch ../glib/gmain.c:3337
> > #17 0x7ffff6c0edfd in g_main_context_dispatch ../glib/gmain.c:4055
> > #18 0x555559726c66 in glib_pollfds_poll ../util/main-loop.c:232
> > #19 0x555559726e43 in os_host_main_loop_wait ../util/main-loop.c:255
> > #20 0x555559727139 in main_loop_wait ../util/main-loop.c:531
> > #21 0x555558fb46fc in qemu_main_loop ../softmmu/runstate.c:722
> > #22 0x555557d45065 in main ../softmmu/main.c:50
> > #23 0x7ffff59611e1 in __libc_start_main (/lib64/libc.so.6+0x281e1)
> > #24 0x555557d44f7d in _start
> (/home/elmarco/src/qemu/build/qemu-system-x86_64+0x27f0f7d)
> >
> > 0x6020003ac4f0 is located 0 bytes inside of 10-byte region
> [0x6020003ac4f0,0x6020003ac4fa)
> > freed by thread T0 here:
> > #0 0x7ffff76870c7 in __interceptor_free (/lib64/libasan.so.6+0xab0c7)
> > #1 0x7ffff6c16d94 in g_free ../glib/gmem.c:199
> > #2 0x5555596caae7 in module_load_one ../util/module.c:234
> > #3 0x5555596ca949 in module_load_one ../util/module.c:225
> > #4 0x5555596ca949 in module_load_one ../util/module.c:225
> > #5 0x5555596cbdf4 in module_load_qom_all ../util/module.c:349
> > #6 0x5555593c6bbc in qmp_qom_list_types ../qom/qom-qmp-cmds.c:114
> > #7 0x5555595576df in qmp_marshal_qom_list_types
> qapi/qapi-commands-qom.c:194
> > #8 0x555559772868 in do_qmp_dispatch_bh ../qapi/qmp-dispatch.c:110
> > #9 0x5555596f8786 in aio_bh_call ../util/async.c:136
> > #10 0x5555596f8e9b in aio_bh_poll ../util/async.c:164
> > #11 0x555559685803 in aio_dispatch ../util/aio-posix.c:381
> > #12 0x5555596fa324 in aio_ctx_dispatch ../util/async.c:306
> > #13 0x7ffff6c0deda in g_main_dispatch ../glib/gmain.c:3337
> > #14 0x7ffff6c0edfd in g_main_context_dispatch ../glib/gmain.c:4055
> > #15 0x555559726c66 in glib_pollfds_poll ../util/main-loop.c:232
> > #16 0x555559726e43 in os_host_main_loop_wait ../util/main-loop.c:255
> > #17 0x555559727139 in main_loop_wait ../util/main-loop.c:531
> > #18 0x555558fb46fc in qemu_main_loop ../softmmu/runstate.c:722
> > #19 0x555557d45065 in main ../softmmu/main.c:50
> > #20 0x7ffff59611e1 in __libc_start_main (/lib64/libc.so.6+0x281e1)
> >
> > Typical C bug...
>
> This commit message isn't really helping explain the problem.
> Rather than this huge trace which readers have to then debug,
> can we actually say what is wrong.
>
> After reading the docs it seems the problem is as follows:
>
> "g_hash_table_add always retains ownership of the pointer
> passed in as the key. Its return status merely indicates
> whether the added entry was new, or replaced an existing
> entry. Thus key must never be freed after this method
> returns."
>
>
Correct, thanks for spelling it out. I can include in the commit and
resend, unless the maintainer (who?.. hmm Paolo?) does it on picking?
thanks
>
> > Fixes: 90629122d2e ("module: use g_hash_table_add()")
> > Cc: qemu-sta...@nongnu.org
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > ---
> > util/module.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/util/module.c b/util/module.c
> > index c65060c167..a2ab0bcdbc 100644
> > --- a/util/module.c
> > +++ b/util/module.c
> > @@ -230,10 +230,11 @@ bool module_load_one(const char *prefix, const
> char *lib_name, bool mayfail)
> > }
> > }
> >
> > - if (!g_hash_table_add(loaded_modules, module_name)) {
> > + if (g_hash_table_contains(loaded_modules, module_name)) {
> > g_free(module_name);
> > return true;
> > }
> > + g_hash_table_add(loaded_modules, module_name);
> >
> > search_dir = getenv("QEMU_MODULE_DIR");
> > if (search_dir != NULL) {
>
> Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
>
>
> 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 :|
>
>
