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 751306 - good plugins: fix some issues found using static analysis tool
good plugins: fix some issues found using static analysis tool
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other Linux
: Normal minor
: 1.5.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-06-22 04:00 UTC by Vineeth
Modified: 2015-06-25 12:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mikmod_reader: prevent possible null pointer dereference (1.06 KB, patch)
2015-06-22 04:01 UTC, Vineeth
none Details | Review
matroska: remove useless check (1.62 KB, patch)
2015-06-22 04:06 UTC, Vineeth
committed Details | Review
lzo: fix memory leak in test code (767 bytes, patch)
2015-06-22 04:08 UTC, Vineeth
none Details | Review
flvdemux: trivial cleanup (1.25 KB, patch)
2015-06-22 04:10 UTC, Vineeth
committed Details | Review
mpegaudioparse: initialize bpf variable (1009 bytes, patch)
2015-06-22 04:14 UTC, Vineeth
committed Details | Review
dcaparse: initialize size variable (967 bytes, patch)
2015-06-22 04:16 UTC, Vineeth
committed Details | Review
mikmod_reader: prevent possible null pointer derference (1.07 KB, patch)
2015-06-22 10:33 UTC, Vineeth
committed Details | Review
lzo: fix memory leak in test code. (753 bytes, patch)
2015-06-22 10:37 UTC, Vineeth
committed Details | Review

Description Vineeth 2015-06-22 04:00:06 UTC
While running static analysis tool(cppcheck) on good plugins, found some potential issues which might create problems later. Attaching patches for the same.
Comment 1 Vineeth 2015-06-22 04:01:54 UTC
Created attachment 305799 [details] [review]
mikmod_reader: prevent possible null pointer dereference
Comment 2 Vineeth 2015-06-22 04:06:27 UTC
Created attachment 305800 [details] [review]
matroska: remove useless check
Comment 3 Vineeth 2015-06-22 04:08:33 UTC
Created attachment 305801 [details] [review]
lzo: fix memory leak in test code
Comment 4 Vineeth 2015-06-22 04:10:47 UTC
Created attachment 305802 [details] [review]
flvdemux: trivial cleanup
Comment 5 Vineeth 2015-06-22 04:14:13 UTC
Created attachment 305803 [details] [review]
mpegaudioparse: initialize bpf variable
Comment 6 Vineeth 2015-06-22 04:16:41 UTC
Created attachment 305804 [details] [review]
dcaparse: initialize size variable
Comment 7 Luis de Bethencourt 2015-06-22 09:41:45 UTC
Review of attachment 305799 [details] [review]:

True. Your code is safer if g_malloc fails, which looks like the original intent.

Could you add an empty line between gst_reader->mik = mik; and the lines that set gst_reader->core?
Comment 8 Tim-Philipp Müller 2015-06-22 09:44:33 UTC
g_malloc will never fail.
Comment 9 Luis de Bethencourt 2015-06-22 09:45:59 UTC
Review of attachment 305800 [details] [review]:

If you look at the gst_matroska_track_free() code in matroska-ids.c. You will see it doesn't check if the context exists or not before it dereferences the structure.

The first line is:
  g_free (track->codec_id);

Are you sure about this patch?
Comment 10 Luis de Bethencourt 2015-06-22 09:49:34 UTC
Review of attachment 305801 [details] [review]:

True. It is good practice to close the file before returning.

No need to set it to NULL though. You can remove that line in the patch.

Notice in the comments at the top of the file it says:
 * This file is part of FFmpeg.

Does the file need any further updates?
Comment 11 Luis de Bethencourt 2015-06-22 09:51:37 UTC
Review of attachment 305802 [details] [review]:

Sorry. I am going to have to reject this change since it is purely coding style changes.

I know the current state of the code is ugly, but we avoid coding style changes since they mess with the history ('git blame') of the file.
Comment 12 Luis de Bethencourt 2015-06-22 09:53:49 UTC
Review of attachment 305803 [details] [review]:

Good catch
Comment 13 Luis de Bethencourt 2015-06-22 09:55:40 UTC
Review of attachment 305804 [details] [review]:

Same as previous patch (mpegaudioparse). Valid fix.
Comment 14 Luis de Bethencourt 2015-06-22 10:00:23 UTC
Review of attachment 305804 [details] [review]:

commit 331fca4dfbf1a398cf5a4792e3b42fa0954301a5
Author: Vineeth TM <vineeth.tm@samsung.com>
Date:   Mon Jun 22 13:13:29 2015 +0900

    mpegaudioparse: initialze bpf variable

    bpf variable might be used in cleanup without being intialized.

    https://bugzilla.gnome.org/show_bug.cgi?id=751306
Comment 15 Luis de Bethencourt 2015-06-22 10:00:47 UTC
Review of attachment 305803 [details] [review]:

commit 331fca4dfbf1a398cf5a4792e3b42fa0954301a5
Author: Vineeth TM <vineeth.tm@samsung.com>
Date:   Mon Jun 22 13:13:29 2015 +0900

    mpegaudioparse: initialze bpf variable

    bpf variable might be used in cleanup without being intialized.

    https://bugzilla.gnome.org/show_bug.cgi?id=751306
Comment 16 Luis de Bethencourt 2015-06-22 10:12:02 UTC
Review of attachment 305802 [details] [review]:

On further thought, this patch should be accepted.
Comment 17 Vineeth 2015-06-22 10:33:51 UTC
Created attachment 305814 [details] [review]
mikmod_reader: prevent possible null pointer derference
Comment 18 Vineeth 2015-06-22 10:37:35 UTC
Created attachment 305815 [details] [review]
lzo: fix memory leak in test code.

The code present in lzo is being used in matroska-read-common.c
probably they have duplicated the code in both ffmpeg and here.
So i guess this fix makes sense :)
Comment 19 Vineeth 2015-06-22 10:40:25 UTC
(In reply to Luis de Bethencourt from comment #9)
> Review of attachment 305800 [details] [review] [review]:
> 
> If you look at the gst_matroska_track_free() code in matroska-ids.c. You
> will see it doesn't check if the context exists or not before it
> dereferences the structure.
> 
> The first line is:
>   g_free (track->codec_id);
> 
> Are you sure about this patch?

the context check is actually useless at that point because, we are entering this code is in the if condition
  if (context->type == 0 || context->codec_id == NULL || (ret != GST_FLOW_OK
          && ret != GST_FLOW_EOS))

where context is already being referenced.
Comment 20 Luis de Bethencourt 2015-06-22 11:16:42 UTC
Review of attachment 305814 [details] [review]:

Thanks! Looks good.
Comment 21 Luis de Bethencourt 2015-06-22 11:19:04 UTC
Review of attachment 305814 [details] [review]:

commit 5828713771e1e65d76d216c5095ebc374da75462
Author: Vineeth T M <vineeth.tm@samsung.com>
Date:   Mon Jun 22 19:30:58 2015 +0900

    mikmod_reader: Possible null pointer dereference:

    gst_reader variable is being used before actually checking if it
    allocated properly

    https://bugzilla.gnome.org/show_bug.cgi?id=751306
Comment 22 Luis de Bethencourt 2015-06-22 11:20:34 UTC
Review of attachment 305802 [details] [review]:

commit 636d47d2d666781e3fc9767a5362939b309d1e01
Author: Vineeth TM <vineeth.tm@samsung.com>
Date:   Mon Jun 22 13:10:02 2015 +0900

    flvdemux: trivial cleanup

    trivial patch to add proper ( while checking for if(G_UNLIKELY())

    https://bugzilla.gnome.org/show_bug.cgi?id=751306
Comment 23 Luis de Bethencourt 2015-06-22 11:22:56 UTC
Review of attachment 305815 [details] [review]:

commit e97df1e0974659695e5d859fd18bd54d81362b84
Author: Vineeth T M <vineeth.tm@samsung.com>
Date:   Mon Jun 22 19:35:57 2015 +0900

    lzo: fix memory leak

    the opened file is not being closed during test, which will result
    in memory leak.

    https://bugzilla.gnome.org/show_bug.cgi?id=751306
Comment 24 Luis de Bethencourt 2015-06-22 11:25:39 UTC
(In reply to Vineeth from comment #19)
> (In reply to Luis de Bethencourt from comment #9)
> > Review of attachment 305800 [details] [review] [review] [review]:
> > 
> > If you look at the gst_matroska_track_free() code in matroska-ids.c. You
> > will see it doesn't check if the context exists or not before it
> > dereferences the structure.
> > 
> > The first line is:
> >   g_free (track->codec_id);
> > 
> > Are you sure about this patch?
> 
> the context check is actually useless at that point because, we are entering
> this code is in the if condition
>   if (context->type == 0 || context->codec_id == NULL || (ret != GST_FLOW_OK
>           && ret != GST_FLOW_EOS))
> 
> where context is already being referenced.

Good point :)

This is why it is important to make detailed commit messages. If not, we might misunderstand the reasons for the patch.
Comment 25 Luis de Bethencourt 2015-06-22 11:31:59 UTC
Review of attachment 305800 [details] [review]:

commit 9a1ed36b7a73120309ea4cacfeba48dc939a0616
Author: Vineeth TM <vineeth.tm@samsung.com>
Date:   Mon Jun 22 13:05:29 2015 +0900

    matroska: remove useless check

    No need to check for context availability while freeing. We are inside
    inside a code block with a condition that dereferences context.
    if (context->type == 0 ...

    https://bugzilla.gnome.org/show_bug.cgi?id=751306
Comment 26 Tim-Philipp Müller 2015-06-25 09:12:15 UTC
Comment on attachment 305814 [details] [review]
mikmod_reader: prevent possible null pointer derference

The correct fix here would have been to remove the if(gst_reader) check, because it doesn't make sense. g_malloc() never returns NULL.

Also, please don't "fix" code that's not in use and not being built, that just adds noise.
Comment 27 Luis de Bethencourt 2015-06-25 12:57:28 UTC
Sorry. I didn't knew g_malloc() never returns NULL.