After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 753701 - discoverer: Few trivial fixes in handling error cases
discoverer: Few trivial fixes in handling error cases
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-08-17 02:13 UTC by Vineeth
Modified: 2015-08-19 07:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
check for info before using it (1.16 KB, patch)
2015-08-17 02:14 UTC, Vineeth
none Details | Review
free context and error (1.11 KB, patch)
2015-08-17 02:18 UTC, Vineeth
committed Details | Review
set GError when info is NULL. (1.09 KB, patch)
2015-08-18 00:12 UTC, Vineeth
committed Details | Review
Check for info and print only error when it is NULL (4.14 KB, patch)
2015-08-18 00:13 UTC, Vineeth
none Details | Review
Check for info and print only error when it is NULL (1.26 KB, patch)
2015-08-18 23:39 UTC, Vineeth
committed Details | Review

Description Vineeth 2015-08-17 02:13:05 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.
Comment 1 Vineeth 2015-08-17 02:14:10 UTC
Created attachment 309386 [details] [review]
check for info before using it
Comment 2 Vineeth 2015-08-17 02:18:57 UTC
Created attachment 309387 [details] [review]
free context and error
Comment 3 Sebastian Dröge (slomo) 2015-08-17 12:05:48 UTC
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
Comment 4 Luis de Bethencourt 2015-08-17 12:25:31 UTC
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
Comment 5 Vineeth 2015-08-17 13:14:45 UTC
(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..
Comment 6 Tim-Philipp Müller 2015-08-17 13:31:01 UTC
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.)
Comment 7 Vineeth 2015-08-18 00:12:47 UTC
Created attachment 309433 [details] [review]
set GError when info is NULL.
Comment 8 Vineeth 2015-08-18 00:13:24 UTC
Created attachment 309434 [details] [review]
Check for info and print only error when it is NULL
Comment 9 Sebastian Dröge (slomo) 2015-08-18 09:15:26 UTC
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
Comment 10 Vineeth 2015-08-18 10:39:36 UTC
(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 :)
Comment 11 Sebastian Dröge (slomo) 2015-08-18 10:49:09 UTC
Also one level less of indentation, which makes it easier to read :)
Comment 12 Vineeth 2015-08-18 23:39:45 UTC
Created attachment 309512 [details] [review]
Check for info and print only error when it is NULL