GNOME Bugzilla – Bug 673897
tsdemux: Fix crash due to NULL pointer access.
Last modified: 2012-05-24 07:18:26 UTC
Created attachment 211820 [details] [review] fix possible NULL pointer access Attached patch fixes a crash I see with tsdemux and reuse of a pipeline. After stop and play a segment_event could not be created, so it was a NULL pointer.
Created attachment 212228 [details] [review] NULL pointer after freeing it. Found another possible crash. currentlist is freed without the pointer being set to NULL afterwards. This leads to double free.
Review of attachment 211820 [details] [review]: ::: gst/mpegtsdemux/tsdemux.c @@ -1504,3 @@ GST_EVENT_SRC (demux->update_segment) = gst_object_ref (demux); } - demux->calculate_update_segment = FALSE; Any reason why you removed this line here? @@ +1517,3 @@ if (demux->update_segment) { GST_DEBUG_OBJECT (stream->pad, "Pushing update segment"); + GST_EVENT_SRC (demux->segment_event) = gst_object_ref (demux); Setting the src doesn't make much sense at all, the pad will be set as src later
Comment on attachment 212228 [details] [review] NULL pointer after freeing it. ...and note the if gst_event_new_whatever() fails something is completely broken already anyway. It must not happen at all and there's no point in checking for this to happen
commit 89ac55bc10fcd7d34aee7c0c766222b669d5a2b8 Author: Julian Scheel <julian@jusst.de> Date: Tue Apr 17 15:44:07 2012 +0200 tsdemux: Set currentlist to NULL after free. This prevents double free of currentlist is some circumstances.
Comment on attachment 211820 [details] [review] fix possible NULL pointer access Sorry, this patch was broken actually. Uploading corrected one.
Created attachment 212265 [details] [review] only assign src when segment_event/update_event was created successfully This fixes a segfault seen with reuse of pipeline with tsdemux.
(In reply to comment #2) > Review of attachment 211820 [details] [review]: > > ::: gst/mpegtsdemux/tsdemux.c > @@ -1504,3 @@ > GST_EVENT_SRC (demux->update_segment) = gst_object_ref (demux); > } > - demux->calculate_update_segment = FALSE; > > Any reason why you removed this line here? No, this was simply wrong, sorry. Uploaded a wrong patch file and didn't see it. The newly uploaded one is correct. > @@ +1517,3 @@ > if (demux->update_segment) { > GST_DEBUG_OBJECT (stream->pad, "Pushing update segment"); > + GST_EVENT_SRC (demux->segment_event) = gst_object_ref (demux); > > Setting the src doesn't make much sense at all, the pad will be set as src > later Then we can simply remove it? Instead of moving it down here? I just moved it as it was in there before and cause the crash.
(In reply to comment #7) > > @@ +1517,3 @@ > > if (demux->update_segment) { > > GST_DEBUG_OBJECT (stream->pad, "Pushing update segment"); > > + GST_EVENT_SRC (demux->segment_event) = gst_object_ref (demux); > > > > Setting the src doesn't make much sense at all, the pad will be set as src > > later > > Then we can simply remove it? Instead of moving it down here? I just moved it > as it was in there before and cause the crash. How did you get a crash here? How could gst_event_new_segment() return NULL?
(In reply to comment #8) > (In reply to comment #7) > > > > @@ +1517,3 @@ > > > if (demux->update_segment) { > > > GST_DEBUG_OBJECT (stream->pad, "Pushing update segment"); > > > + GST_EVENT_SRC (demux->segment_event) = gst_object_ref (demux); > > > > > > Setting the src doesn't make much sense at all, the pad will be set as src > > > later > > > > Then we can simply remove it? Instead of moving it down here? I just moved it > > as it was in there before and cause the crash. > > How did you get a crash here? How could gst_event_new_segment() return NULL? This was simply reproduced with an endless loop of stream stop/start events on the same pipeline. I will try to revert the patch and see if the issue appears again then later today. It might have been triggered by some other error, which might fixed in the meantime. There were some memory corruptions, etc. I will try to post an update on this in the afternoon.
Can you provide logs ?
I can not reproduce the bug with current git anymore. So either it's fixed by some other cleanups or it was a phantom issue anyway. I think this bug can be closed now.
Closing for now. Re-open if issue can be reproduced with logs.