GNOME Bugzilla – Bug 760565
test failure due to double free when goa is not available
Last modified: 2016-02-29 11:33:47 UTC
when we build grilo-plugins on the ubuntu buildd's GOA is not available (since there is no X session) as such the local_metadata_test fails to connect to GOA which results in the test case failing with a double free. valgrind output: /lua-factory/video-title-parsing/resolve/episodes: OK /lua-factory/video-title-parsing/resolve/title-override: OK ==22864== Invalid free() / delete / delete[] / realloc() ==22864== at 0x4C2CE2B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==22864== by 0x9EE7367: grl_lua_goa_data_free (grl-lua-factory.c:771) ==22864== by 0x9EE7427: grl_lua_factory_plugin_deinit (grl-lua-factory.c:289) ==22864== by 0x4E45251: grl_plugin_unload (in /usr/lib/x86_64-linux-gnu/libgrilo-0.2.so.1.9.0) ==22864== by 0x4E45977: ??? (in /usr/lib/x86_64-linux-gnu/libgrilo-0.2.so.1.9.0) ==22864== by 0x4E47CE9: grl_registry_shutdown (in /usr/lib/x86_64-linux-gnu/libgrilo-0.2.so.1.9.0) ==22864== by 0x4E54A2A: grl_deinit (in /usr/lib/x86_64-linux-gnu/libgrilo-0.2.so.1.9.0) ==22864== by 0x4012A7: main (test_local_metadata.c:204) ==22864== Address 0x9c44f90 is 0 bytes inside a block of size 88 free'd ==22864== at 0x4C2CE2B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==22864== by 0x9EE7367: grl_lua_goa_data_free (grl-lua-factory.c:771) ==22864== by 0x9EE97CD: grl_lua_factory_goa_init (grl-lua-factory.c:795) ==22864== by 0x5E46F22: ??? (in /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.4703.0) ==22864== by 0x5E46F58: ??? (in /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.4703.0) ==22864== by 0x530CFC9: g_main_context_dispatch (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4703.0) ==22864== by 0x530D36F: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4703.0) ==22864== by 0x530D41B: g_main_context_iteration (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4703.0) ==22864== by 0x4E4AF4C: grl_wait_for_async_operation_complete (in /usr/lib/x86_64-linux-gnu/libgrilo-0.2.so.1.9.0) ==22864== by 0x4E4F8D7: grl_source_resolve_sync (in /usr/lib/x86_64-linux-gnu/libgrilo-0.2.so.1.9.0) ==22864== by 0x4017BD: get_show_for_title (test_local_metadata.c:64) ==22864== by 0x4017BD: test_episodes (test_local_metadata.c:122) ==22864== by 0x533270A: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4703.0) ==22864== Block was alloc'd at ==22864== at 0x4C2BBCF: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==22864== by 0x5312698: g_malloc (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4703.0) ==22864== by 0x532B48E: g_strdup (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4703.0) ==22864== by 0x9EE933C: handle_goa_sources (grl-lua-factory.c:991) ==22864== by 0x9EE933C: grl_lua_factory_plugin_init (grl-lua-factory.c:204) ==22864== by 0x4E451A7: grl_plugin_load (in /usr/lib/x86_64-linux-gnu/libgrilo-0.2.so.1.9.0) ==22864== by 0x4E464F2: ??? (in /usr/lib/x86_64-linux-gnu/libgrilo-0.2.so.1.9.0) ==22864== by 0x4E46E2D: ??? (in /usr/lib/x86_64-linux-gnu/libgrilo-0.2.so.1.9.0) ==22864== by 0x4E487A1: grl_registry_load_all_plugins (in /usr/lib/x86_64-linux-gnu/libgrilo-0.2.so.1.9.0) ==22864== by 0x401251: test_setup (test_local_metadata.c:35) ==22864== by 0x401251: main (test_local_metadata.c:197) ==22864==
Created attachment 318941 [details] [review] lua-factory: guard againt double free of GOA data This fixes test suite failure under Ubuntu buildd's where there is not running X session.
Comment on attachment 318941 [details] [review] lua-factory: guard againt double free of GOA data Doesn't seem to be the right fix. If data is NULL (which I think it is possible) you accessing it now. If you want to avoid double-free you must free then set the pointer to NULL... then the current if (data == NULL) should work just fine
Also, if you are not using goa you can disable it in the configure options!
Review of attachment 318941 [details] [review]: I forgot to mark the patch..
we use GOA, but its not available in the build environment, since there is not running X session. I agree its probably not the exact fix. However there seem to be other data objects in that structure, unrelated to GOA, so can't just null it when goa_data_init fails? perhaps if (data == NULL && data->client == NULL) would be better
just to clarify a bit, its the runtime part of GOA that is not available in the builders. Obviously all the libs etc are there.
(In reply to darkxst from comment #5) > we use GOA, but its not available in the build environment, since there is > not running X session. > > I agree its probably not the exact fix. However there seem to be other data > objects in that structure, unrelated to GOA, so can't just null it when > goa_data_init fails? The GOA struct is not necessary for Lua-Factory and the lua plugins and the possible reason for the double free is a pointer misbehaving as not being set to NULL IMHO. > > perhaps if (data == NULL && data->client == NULL) would be better And this probably could lead to memory leak? GoaData is alloc with g_new0 but it does not seem to be free anywhere. So i guess it is even leaking already. Maybe grl_lua_goa_data_free should receive (GrlLuaGoaData **) and set the pointer to NULL after freeing its data. I can't reproduce it here so if you can find a better fix would be great :)
ps: i guess you meant > if (data == NULL || data->client == NULL)
Created attachment 319070 [details] [review] lua-factory: guard againt double free of GOA data This fixes test suite failure under Ubuntu buildd's where there is not running X session, so goa_client_new_finish() fails with an error. What was happening here was that when goa_client_new_finish() fails we free up the data, but keep a reference in lua_init-sources which later triggers a second free.
Created attachment 319071 [details] [review] lua-factory: Fix memory leak of hash table Just something I noticed while poking around in valgrind
Review of attachment 319071 [details] [review]: great! just the summary is not correct: "lua-factory: Fix memory leak of hash table" It is a list, not a hash table
Created attachment 319082 [details] [review] lua-factory: Fix memory leak of list lol, how did I write that! code says it all!
bttw I do have commit rights, so you don't need to push patches for me, unless you really like to do that!
Review of attachment 319070 [details] [review]: Yes, this seems to be the right fix! Thanks for the patches! Just a small comments.. also the commit log could be improved (it isn't guard anymore) but I'm fine with it as it is as well. cheers! ::: src/lua-factory/grl-lua-factory.c @@ +262,2 @@ return source_loaded; } You've removed extra line that separated these functions @@ +778,3 @@ g_clear_pointer (&data->sources, g_hash_table_destroy); + g_free (data); + no need extra line here
Review of attachment 319082 [details] [review]: yes!
Created attachment 319083 [details] [review] lua-factory: avoid double free of GOA data This fixes test suite failure under Ubuntu buildd's where there is not running X session, so goa_client_new_finish() fails with an error.
Review of attachment 319083 [details] [review]: Yes! I did not know that you had commit access. Feel free to push it ;) Thanks for your patches!
Attachment 319082 [details] pushed as 26217f9 - lua-factory: Fix memory leak of list Attachment 319083 [details] pushed as 78cf2dd - lua-factory: avoid double free of GOA data
ok to push these to the 0.2.x also? that is what we will likely ship in Ubuntu 16.04 at this stage
(In reply to darkxst from comment #19) > ok to push these to the 0.2.x also? that is what we will likely ship in > Ubuntu 16.04 at this stage Sure. Just to let you know that It is not planned to have another 0.2.x release in the future. https://mail.gnome.org/archives/grilo-list/2015-December/msg00007.html
yes, I would prefer to move to 0.3 (and may still) but that will mean backporting other patches to GNOME 3.18
*** Bug 762837 has been marked as a duplicate of this bug. ***