GNOME Bugzilla – Bug 753701
discoverer: Few trivial fixes in handling error cases
Last modified: 2015-08-19 07:21:19 UTC
This handles 2 fixes in discoverer 1) While using gst_discoverer_discover_uri, there are chances that info will be returned as NULL in error cases. Passing NULL values to gst_discoverer_info*** APIs results in assertion failures like below CRITICAL **: gst_discoverer_info_get_result: assertion 'G_TYPE_CHECK_INSTANCE_TYPE((info), (gst_discoverer_info_get_type ()))' failed 2) During error cases, for g_option_context_parse and gst_discoverer_new, err will be allocated but not freed resulting in memory leaks.
Created attachment 309386 [details] [review] check for info before using it
Created attachment 309387 [details] [review] free context and error
Review of attachment 309386 [details] [review]: ::: tools/gst-discoverer.c @@ +492,3 @@ info = gst_discoverer_discover_uri (dc, uri, &err); + if (info) { + print_info (info, err); This doesn't seem like the correct fix. If there was an error, you might not get a info but err might contain useful information. print_info() should probably just print the error if info==NULL
To keep a reference of things. commit 6ee8b22c403bb15407ca012e54cf463c1b24ed13 Author: Vineeth TM <vineeth.tm@samsung.com> Date: Mon Aug 17 11:18:25 2015 +0900 discoverer: free context and error during failures When g_option_context_parse or gst_discoverer_new fails, then there will be memory leaks for ctx and err variables. Free'ing the same. https://bugzilla.gnome.org/show_bug.cgi?id=753701
(In reply to Sebastian Dröge (slomo) from comment #3) > Review of attachment 309386 [details] [review] [review]: > > ::: tools/gst-discoverer.c > @@ +492,3 @@ > info = gst_discoverer_discover_uri (dc, uri, &err); > + if (info) { > + print_info (info, err); > > This doesn't seem like the correct fix. If there was an error, you might not > get a info but err might contain useful information. print_info() should > probably just print the error if info==NULL Actually in the function gst_discoverer_discover_uri, when NULL is being returned, error is not getting filled... Only when current info is present, then the actual discovery starts and if it fails then error will be allocated... So when info is NULL err will never be allocated or filled..
That sounds like a bug. GError should always be set if a NULL info is returned (unless it's a g_return_if_fail()). (This trips up coverity and other static analyzers of course, because they sense that there's a code path [g_return_if_fail] that doesn't set it and then bad things happen, but that's par for the course then.)
Created attachment 309433 [details] [review] set GError when info is NULL.
Created attachment 309434 [details] [review] Check for info and print only error when it is NULL
Review of attachment 309434 [details] [review]: ::: tools/gst-discoverer.c @@ +383,3 @@ { + if (info) { + GstDiscovererResult result = gst_discoverer_info_get_result (info); Instead of moving all the code around, just do if (!info) { ... return; } // old code
(In reply to Sebastian Dröge (slomo) from comment #9) > Review of attachment 309434 [details] [review] [review]: > > ::: tools/gst-discoverer.c > @@ +383,3 @@ > { > + if (info) { > + GstDiscovererResult result = gst_discoverer_info_get_result (info); > > Instead of moving all the code around, just do > > if (!info) { > ... > return; > } > > // old code Yes i thought of this.. But for this i will have to move result and sinfo before that and later on get the result from info_get_result But yeah lesser changes in lines than moving the whole thing around :)
Also one level less of indentation, which makes it easier to read :)
Created attachment 309512 [details] [review] Check for info and print only error when it is NULL