GNOME Bugzilla – Bug 751306
good plugins: fix some issues found using static analysis tool
Last modified: 2015-06-25 12:57:28 UTC
While running static analysis tool(cppcheck) on good plugins, found some potential issues which might create problems later. Attaching patches for the same.
Created attachment 305799 [details] [review] mikmod_reader: prevent possible null pointer dereference
Created attachment 305800 [details] [review] matroska: remove useless check
Created attachment 305801 [details] [review] lzo: fix memory leak in test code
Created attachment 305802 [details] [review] flvdemux: trivial cleanup
Created attachment 305803 [details] [review] mpegaudioparse: initialize bpf variable
Created attachment 305804 [details] [review] dcaparse: initialize size variable
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?
g_malloc will never fail.
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?
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?
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.
Review of attachment 305803 [details] [review]: Good catch
Review of attachment 305804 [details] [review]: Same as previous patch (mpegaudioparse). Valid fix.
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
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
Review of attachment 305802 [details] [review]: On further thought, this patch should be accepted.
Created attachment 305814 [details] [review] mikmod_reader: prevent possible null pointer derference
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 :)
(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.
Review of attachment 305814 [details] [review]: Thanks! Looks good.
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
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
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
(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.
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 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.
Sorry. I didn't knew g_malloc() never returns NULL.