Bug 751000 – oggdemux: crash with validate.http.media_check.vorbis_theora_1_ogg

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 751000 - oggdemux: crash with validate.http.media_check.vorbis_theora_1_ogg


Summary:	oggdemux: crash with validate.http.media_check.vorbis_theora_1_ogg


Status:	RESOLVED FIXED

Product:	GStreamer
Classification:	Platform
Component:	gst-plugins-base
Version:	git master
Hardware:	Other Linux

Importance:	Normal major
Target Milestone:	1.5.2
Assigned To:	GStreamer Maintainers
QA Contact:	GStreamer Maintainers

URL:
Whiteboard:

Depends on:
Blocks:

Reported:	2015-06-15 14:10 UTC by Guillaume Desmottes
Modified:	2015-06-22 12:10 UTC

See Also:
GNOME target:	---
GNOME version:	---

Attachments
oggdemux: set building_chain to NULL when clearing chains (930 bytes, patch) 2015-06-15 14:12 UTC, Guillaume Desmottes	none	Details \| Review
oggdemux: set building_chain to NULL when deactivating chain (968 bytes, patch) 2015-06-16 08:55 UTC, Guillaume Desmottes	committed	Details \| Review

Description Guillaume Desmottes 2015-06-15 14:10:16 UTC

The validate.http.media_check.vorbis_theora_1_ogg test is raising a crash.

+ Trace 235169

#0 __GI_raise
at ../sysdeps/unix/sysv/linux/raise.c line 55
#1 __GI_abort
at abort.c line 89
#2 __libc_message
at ../sysdeps/posix/libc_fatal.c line 175
#3 __GI___libc_free
#4 __GI___libc_free
at malloc.c line 3841
#5 __GI___libc_free
at malloc.c line 2951
#6 g_free
at gmem.c line 190
#7 array_free
#8 g_array_free
at garray.c line 346
#9 gst_ogg_chain_free
at gstoggdemux.c line 2057
#10 gst_ogg_demux_finalize
at gstoggdemux.c line 2276
#11 g_object_unref
at gobject.c line 3170
#12 gst_object_unref
at gstobject.c line 282
#13 gst_decode_chain_free_internal
at gstdecodebin2.c line 3288
#14 gst_decode_bin_change_state
at gstdecodebin2.c line 3308
#15 gst_decode_bin_change_state
at gstdecodebin2.c line 4968
#16 gst_element_change_state
at gstelement.c line 2604
#17 gst_element_set_state_func
at gstelement.c line 2560
#18 gst_bin_change_state_func
at gstbin.c line 2328
#19 gst_bin_change_state_func
at gstbin.c line 2676
#20 gst_uri_decode_bin_change_state
at gsturidecodebin.c line 2752
#21 gst_element_change_state
at gstelement.c line 2604
#22 gst_element_set_state_func
at gstelement.c line 2560
#23 gst_bin_change_state_func
at gstbin.c line 2328
#24 gst_bin_change_state_func
at gstbin.c line 2676
#25 gst_element_change_state
at gstelement.c line 2604
#26 gst_element_set_state_func
at gstelement.c line 2560
#27 discoverer_cleanup
at gstdiscoverer.c line 1534
#28 gst_discoverer_discover_uri
at gstdiscoverer.c line 2134
#29 gst_media_descriptor_writer_new_discover
at media-descriptor-writer.c line 527
#30 main
at gst-validate-media-check.c line 97

Comment 1 Guillaume Desmottes 2015-06-15 14:12:00 UTC

Created attachment 305304 [details] [review]
oggdemux: set building_chain to NULL when clearing chains

All the chains just have been destroyed, including building_chain which is now
invalid. Unsetting ogg->building_chain will prevent a double free crash when
the demuxer is being finalized.

Comment 2 Thiago Sousa Santos 2015-06-15 14:56:04 UTC

Review of attachment 305304 [details] [review]:

::: ext/ogg/gstoggdemux.c
@@ +4910,3 @@
   }
   ogg->chains = g_array_set_size (ogg->chains, 0);
+  ogg->building_chain = NULL;

Are you sure that the building_chain has been freed here? Is it guaranteed that it is in the chains array?

I couldn't find where it was added to the array in the code.

Comment 3 Vineeth 2015-06-16 00:36:35 UTC

Review of attachment 305304 [details] [review]:

I guess it would make more sense to add ogg->building_chain = NULL; in the function 
gst_ogg_demux_deactivate_current_chain()

where the current chain is being deactivated.

  GstOggChain *chain = ogg->current_chain;

  if (!ogg->pullmode) {
    gst_ogg_chain_free (chain);
  }


So the change can be something like
  if (!ogg->pullmode) {
    gst_ogg_chain_free (chain);
    ogg->current_chain = NULL;
  }

Comment 4 Guillaume Desmottes 2015-06-16 08:55:22 UTC

You're right that's cleaner this way.

Comment 5 Guillaume Desmottes 2015-06-16 08:55:38 UTC

Created attachment 305379 [details] [review]
oggdemux: set building_chain to NULL when deactivating chain

The chain is about to be invalidated so we shouldn't keep it around.
Prevent a double free crash when the demuxer is being finalized.

Comment 6 Sebastian Dröge (slomo) 2015-06-22 12:10:16 UTC

commit a5dcce98aa0c77cf6c31aeb7e2cb0c12abeec1ba
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Mon Jun 15 16:08:10 2015 +0200

    oggdemux: set building_chain to NULL when deactivating chain
    
    The chain is about to be invalidated so we shouldn't keep it around.
    Prevent a double free crash when the demuxer is being finalized.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=751000