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 741751 - mssdemux: pass correct type through vararg function to avoid potential crash
mssdemux: pass correct type through vararg function to avoid potential crash
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.4.5
Other Linux
: Normal critical
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 755033 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-12-19 10:03 UTC by Rajat Verma
Modified: 2015-09-16 08:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (1016 bytes, patch)
2014-12-19 10:05 UTC, Rajat Verma
none Details | Review
Proposed patch (1018 bytes, patch)
2014-12-19 11:20 UTC, Rajat Verma
none Details | Review
Proposed patch (1.01 KB, patch)
2015-01-07 06:18 UTC, Rajat Verma
none Details | Review

Description Rajat Verma 2014-12-19 10:03:12 UTC
In gstmssmanifest.c, _GstMssStreamQuality::bitrate is specified as of type "guint64" but it is set incorrectly in caps structure using "G_TYPE_INT". 

typedef struct _GstMssStreamQuality
{
  xmlNodePtr xmlnode;

  gchar *bitrate_str;
  guint64 bitrate;
} GstMssStreamQuality;

_gst_mss_stream_audio_caps_from_qualitylevel_xml (GstMssStreamQuality * q)
  ...
  if (q->bitrate)
    gst_structure_set (structure, "bitrate", G_TYPE_INT, q->bitrate, NULL);
  ...
Comment 1 Rajat Verma 2014-12-19 10:05:51 UTC
Created attachment 293033 [details] [review]
Proposed patch
Comment 2 Rajat Verma 2014-12-19 11:20:25 UTC
Created attachment 293039 [details] [review]
Proposed patch

Corrected e-mail id
Comment 3 Thiago Sousa Santos 2014-12-22 13:59:19 UTC
Thanks for the fix but it seems that faac (other element that also deals with AAC) uses bitrate as an int and not as int64. For being uniform it would be better to have the bitrate as an INT in AAC GstStructure.

Did this cause any issue to you or it just draw your attention because of the type difference?
Comment 4 Rajat Verma 2015-01-06 04:31:10 UTC
I was testing mssdemux support in gstreamer and encountered a crash because of this.
Comment 5 Rajat Verma 2015-01-06 05:06:11 UTC
Microsoft media for Smooth Streaming used is ElephantsDreamH.264720p, which was downloaded from Microsoft's site itself.


Below is command and stack trace for your reference:

root@Rajat:~# gdb --args gst-launch-1.0 playbin uri=file:///media/SDK2Streams/AdaptiveStreaming/ElephantsDreamH.264720p/ElephantsDream.ismc
GNU gdb (GDB) STMicroelectronics/Linux Base 7.6-49 [build Jul  2 2014]
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "arm-cortex-linux-gnueabi".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /usr/bin/gst-launch-1.0...done.
(gdb) r
Starting program: /usr/bin/gst-launch-1.0 playbin uri=file:///media/SDK2Streams/AdaptiveStreaming/ElephantsDreamH.264720p/ElephantsDream.ismc
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/libthread_db.so.1".
Setting pipeline to PAUSED ...
[New Thread 0x76965470 (LWP 3420)]
Pipeline is PREROLLING ...
[New Thread 0x76165470 (LWP 3421)]
[New Thread 0x75648470 (LWP 3422)]

(gst-launch-1.0:3417): GLib-GObject-WARNING **: ../../gobject/gtype.c:4204: type id `0' is invalid

(gst-launch-1.0:3417): GLib-GObject-WARNING **: can't peek value table for type `<invalid>' which is not currently referenced

Program received signal SIGSEGV, Segmentation fault.

Thread 1989563504 (LWP 3420)

  • #0 gst_structure_set_valist_internal
    at gststructure.c line 605
  • #1 gst_structure_set
    at gststructure.c line 635
  • #2 _gst_mss_stream_audio_caps_from_qualitylevel_xml
    at ../../../ext/smoothstreaming/gstmssmanifest.c line 630
  • #3 gst_mss_stream_get_caps
    at ../../../ext/smoothstreaming/gstmssmanifest.c line 739
  • #4 gst_mss_demux_expose_stream
    at ../../../ext/smoothstreaming/gstmssdemux.c line 823
  • #5 gst_mss_demux_process_manifest
    at ../../../ext/smoothstreaming/gstmssdemux.c line 936
  • #6 gst_mss_demux_event
    at ../../../ext/smoothstreaming/gstmssdemux.c line 505
  • #7 gst_pad_send_event_unchecked
    at gstpad.c line 5135
  • #8 gst_pad_push_event_unchecked
    at gstpad.c line 4833
  • #9 push_sticky
    at gstpad.c line 3443
  • #10 events_foreach
    at gstpad.c line 570
  • #11 check_sticky
    at gstpad.c line 3499
  • #12 gst_pad_push_event
    at gstpad.c line 4952
  • #13 gst_type_find_element_loop
    at gsttypefindelement.c line 1158
  • #14 gst_task_func
    at gsttask.c line 316
  • #15 g_thread_pool_thread_proxy
    from /usr/lib/libglib-2.0.so.0
  • #16 g_thread_proxy
    from /usr/lib/libglib-2.0.so.0
  • #17 start_thread
    from /lib/libpthread.so.0
  • #18 ??
    from /lib/libc.so.6
  • #19 ??
    from /lib/libc.so.6

Comment 6 Tim-Philipp Müller 2015-01-06 09:23:02 UTC
This issue looks valid. We're passing 64-bit as vararg argument, but only specify G_TYPE_INT, which means on platforms where sizeof(int) != 64 we're not reading enough bits off the stack and then everything goes wrong from there.

Question is whether the fix should be:

  ..., "bitrate", G_TYPE_INT, (int) q->bitrate, ...

or (as proposed)

  ..., "bitrate", G_TYPE_INT64, q->bitrate, ...

Depends on who reads this later I suppose.
Comment 7 Thiago Sousa Santos 2015-01-06 11:06:07 UTC
I agree with Tim here. We should just go and put an explicit cast to int when setting it in the structure.

Rajat, can you provide a patch for that?
Comment 8 Thiago Sousa Santos 2015-01-06 11:08:55 UTC
(In reply to comment #7)
> I agree with Tim here. We should just go and put an explicit cast to int when
> setting it in the structure.

I mean, we need to be uniform with the other elements expect. And as far as I checked it is used as an INT

> 
> Rajat, can you provide a patch for that?
Comment 9 Rajat Verma 2015-01-06 11:43:36 UTC
> Rajat, can you provide a patch for that?

ok, I will prepare a patch for it
Comment 10 Rajat Verma 2015-01-07 06:18:13 UTC
Created attachment 294004 [details] [review]
Proposed patch
Comment 11 Rajat Verma 2015-01-07 08:08:18 UTC
restored the severity as set by Tim-Philipp. I had reset it mistakenly.
Comment 12 Tim-Philipp Müller 2015-01-07 10:23:12 UTC
Thanks for the bug report and the patch. Pushed to master:

commit de246c6741d70077a06ea223297c5ebc57ba471c
Author: Rajat Verma <rajat.verma@st.com>
Date:   Wed Jan 7 11:31:30 2015 +0530

    mssdemux: fix crash while setting bitrate in caps structure
    
    q->bitrate is a guint64, but G_TYPE_INT may read fewer bits
    off the stack, and if we pass more then the NULL sentinel
    may not be found at the right place, which in turn might
    lead to crashes.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=741751
Comment 13 Tim-Philipp Müller 2015-09-16 08:41:41 UTC
*** Bug 755033 has been marked as a duplicate of this bug. ***