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.

+ Trace 234507

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. ***