GNOME Bugzilla – Bug 751528
mpegdemux: Fix a prevent defect which dereference null return value
Last modified: 2015-08-16 13:41:36 UTC
I found a prevent defect using static analysis tool.
Created attachment 306140 [details] [review] It is a prevent defect which dereference null return value. 'gst_flups_demux_get_stream' function could return NULL when it is unknown stream. So I added NULL check.
Created attachment 306141 [details] [review] I fixed a typing error in patch.
Created attachment 306142 [details] [review] Sorry, I attached the patch again. Fix a prevent defect which dereference null return value. 'gst_ps_demux_get_stream' function could return NULL when it is unknown stream. So I added NULL check.
Comment on attachment 306142 [details] [review] Sorry, I attached the patch again. >From 261d6534f23958d22de025a29b5d3aa7b906ac45 Mon Sep 17 00:00:00 2001 >From: Sangkyu Park <sk1122.park@samsung.com> >Date: Fri, 26 Jun 2015 15:58:25 +0900 >Subject: [PATCH] mpegdemux: Minor clean-up > >Fix a prevent defect which dereference null return value. >'gst_ups_demux_get_stream' function could return NULL when it is unknown stream. > >https://bugzilla.gnome.org/show_bug.cgi?id=751528 >--- > gst/mpegdemux/gstmpegdemux.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > mode change 100644 => 100755 gst/mpegdemux/gstmpegdemux.c > >diff --git a/gst/mpegdemux/gstmpegdemux.c b/gst/mpegdemux/gstmpegdemux.c >old mode 100644 >new mode 100755 >index ab9b364..54551c9 >--- a/gst/mpegdemux/gstmpegdemux.c >+++ b/gst/mpegdemux/gstmpegdemux.c >@@ -824,7 +824,7 @@ gst_ps_demux_handle_dvd_event (GstPsDemux * demux, GstEvent * event) > > g_snprintf (cur_stream_name, 32, "audio-%d-language", i); > lang_code = gst_structure_get_string (structure, cur_stream_name); >- if (lang_code) { >+ if (lang_code && temp) { > GstTagList *list = temp->pending_tags; > > if (!list) >@@ -855,6 +855,8 @@ gst_ps_demux_handle_dvd_event (GstPsDemux * demux, GstEvent * event) > /* Retrieve the subpicture stream to force pad creation */ > temp = gst_ps_demux_get_stream (demux, 0x20 + stream_id, > ST_PS_DVD_SUBPICTURE); >+ if (temp == NULL) >+ continue; > > g_snprintf (cur_stream_name, 32, "subpicture-%d-language", i); > lang_code = gst_structure_get_string (structure, cur_stream_name); >-- >1.7.9.5 >
Hi you can mark the previous patches as obsolete, so that there will be no duplications
Created attachment 306143 [details] [review] Sorry, it has a typing error in patch so I updated it again.
Created attachment 306144 [details] [review] I updated, Thanks :)
It is a prevent defect which dereference null return value. 'gst_ps_demux_get_stream' function could return NULL when it is unknown stream. So I added NULL check.
Review of attachment 306144 [details] [review]: ::: gst/mpegdemux/gstmpegdemux.c @@ +825,3 @@ g_snprintf (cur_stream_name, 32, "audio-%d-language", i); lang_code = gst_structure_get_string (structure, cur_stream_name); + if (lang_code && temp) { it might be better to break for default case and add if (temp == NULL) continue; before line 825
Created attachment 306146 [details] [review] I updated it
Created attachment 306147 [details] [review] Sorry I updated it again. I know that must recopy commit tag also when making patch again.
I'm beginner about contributing open source. sorry I have made many mistakes.
Two things. - Don't compare to NULL, when you can directly do if (!temp). - In GStreamer's coding style we add a space between the 'if' and the '('. Logic of patch is good. The code must check if gst_ps_demux_get_stream () returned NULL before dereferencing temp. Change it to: if (!temp) continue;
Sangkyu, Tim pointed out that the norm is to compare to NULL to make it clear a pointer is being checked and not a boolean. I went ahead and fixed your space between if and '(' and submitted. Thank you for your contribution :)
Review of attachment 306147 [details] [review]: commit 0ca354eb102d883d0bc4cb9762c8911ccf698ffc Author: Sangkyu Park <sk1122.park@samsung.com> Date: Fri Jun 26 15:58:25 2015 +0900 mpegdemux: check pointer before dereferencing gst_ps_demux_get_stream() could return NULL when it is unknown stream, check this hasn't happened before dereferencing the returned pointer. https://bugzilla.gnome.org/show_bug.cgi?id=751528
(clean-up is something that doesn't change behaviour :))
Vineeth, Luis and Tim-philipp, Thanks for your advice :) I will check one more next time for no mistake.