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 655727 - rtp: segfault in gst_rtcp_packet_get_rb()
rtp: segfault in gst_rtcp_packet_get_rb()
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 1.0.10
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-08-01 15:54 UTC by anthony
Modified: 2013-08-28 13:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Segfault gst_rtcp_packet_get_rb (3.99 KB, patch)
2011-08-01 15:54 UTC, anthony
needs-work Details | Review
Bug fix in gst_rtcp_packet_get_rb (4.07 KB, patch)
2011-08-03 14:00 UTC, anthony
none Details | Review
Bug-in-gst_rtcp_packet_add_rb-overflow-data (1.29 KB, patch)
2011-08-10 13:50 UTC, anthony
none Details | Review

Description anthony 2011-08-01 15:54:43 UTC
Created attachment 192991 [details] [review]
Segfault gst_rtcp_packet_get_rb

hello,


I found a segfault in the function gst_rtcp_packet_get_rb at line 1002, when GST_READ_UINT32_BE (data) is called;

it seems that offsetting the pointer to data can cause a buffer overflow (e.g. *data += 24) in some conditions.

I noticed that in my case (using and Elphel camera where buffer size is always 28) packet->type is never GST_RTCP_TYPE_RR, which causes a systematic 24 offset;
added to other offset operations, the total offset sometimes is > buffer->size, which causes (IMO) the GST_READ_UINT32_BE segfault.

How should this bug be fixed ? Am i correct in analyzing the behaviour ? Any ideas ?

Pipeline :

gst-launch rtspsrc location=camera protocols=0x00000001 latency=50 ! rtpjpegdepay ! jpegdec ! videoscale ! videorate ! "video/x-raw-yuv, format=(fourcc)I420, width=(int)2592, height=(int)816, framerate=(fraction)25/1" ! jpegenc ! matroskamux ! filesink location=test_jpeg.mkv

Details (pipeline, full backtrace) here:  http://pastebin.com/g3t8YLBg

For the moment, i made a patch for correct this bug.
Comment 1 Sebastian Dröge (slomo) 2011-08-03 08:14:34 UTC
Could you please attach a patch with correct author information and without unrelated whitespace changes? Also the added check for the size looks suspicious, can you really assume that the offset is 4 if the packet size is too small?
Comment 2 anthony 2011-08-03 09:10:17 UTC
I will correct the patch. 
In the gst_rtcp_packet_get_rb function the offset can be 28 or 48. 
There is no check for buffer under 28, so i will add one. 
If the buffer is 28 the offset must be 4 and not 24 that was my problem.
i can try to implement a functionnality that calculate the offset with buffer from 28 to 48.
Comment 3 anthony 2011-08-03 09:37:40 UTC
I will correct the patch. 
In the gst_rtcp_packet_get_rb function the offset can be 28 or 48. 
There is no check for buffer under 28, so i will add one. 
If the buffer is 28 the offset must be 4 and not 24 that was my problem.
i can try to implement a functionnality that calculate the offset with buffer from 28 to 48.
Comment 4 anthony 2011-08-03 14:00:14 UTC
Created attachment 193170 [details] [review]
Bug fix in gst_rtcp_packet_get_rb

I followed the official documentation (http://gstreamer.freedesktop.org/wiki/SubmittingPatches) which states that gst-indent should be used on patches before submitting (which i did), which removes all whitespaces. git refuses commits if whitespaces aren't removed. What should i do ? I updated the author information.
Comment 5 anthony 2011-08-10 13:50:28 UTC
Created attachment 193548 [details] [review]
Bug-in-gst_rtcp_packet_add_rb-overflow-data

i made patch without unrelated whitespace changes.
I think the patch is good now.
Comment 6 Wim Taymans 2013-02-07 13:54:50 UTC
This patch looks weird, what is wrong with the RTCP packet? can you maybe make a hexdump of it?
Comment 7 Tim-Philipp Müller 2013-08-26 19:59:47 UTC
Wim, you pushed:

 commit ca1dac69826d47d6700fd6c22d7ffbb3f638dfaa
 Author: Wim Taymans <wim.taymans@collabora.co.uk>
 Date:   Mon Aug 26 11:47:40 2013 +0200

    rtcpbuffer: do additional packet checks
    
    Check the packet size and avoid crashing on malformed packets.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=655727

- can this be closed then?
Comment 8 Wim Taymans 2013-08-27 07:30:46 UTC
yes
Comment 9 Tim-Philipp Müller 2013-08-27 11:26:04 UTC
Cherry-picked into 1.0 as well.