GNOME Bugzilla – Bug 650470
RTP: Some depayloaders break with malformed rtp packets
Last modified: 2011-05-19 08:29:47 UTC
Created attachment 188021 [details] [review] Patch that solves the described problem I've found that some depayloaders fall down in a segmentation fault when the received rtp packet is malformed. I've tested with PortGo softphone for Windows. The first rtp packet that this program emits is malformed (according to wireshark). When this packed is received, the depayload tries to find the payload buffer of the packet but is not there and the functions gst_rtp_buffer_get_payload_buffer and gst_rtp_buffer_get_payload_subbuffer return NULL and print this line: ** (GstreamerMediaServer:8057): WARNING **: offset=0 should be less then plen=0 The problem is that some depayloaders do not check if the returned values is NULL and they try to use it. I attach a patch that solves the problem for all the depayloaders that didn't do the check.
Review of attachment 188021 [details] [review]: ::: gst/rtp/gstrtpac3depay.c @@ +196,3 @@ + if (outbuf) + GST_DEBUG_OBJECT (rtpac3depay, "pushing buffer of size %d", + GST_BUFFER_SIZE (outbuf)); If outbuf==NULL, shouldn't there be some kind of error message instead? ::: gst/rtp/gstrtpbvdepay.c @@ +166,3 @@ outbuf = gst_rtp_buffer_get_payload_buffer (buf); + if (marker && outbuf) { Same here ::: gst/rtp/gstrtpg722depay.c @@ +237,3 @@ marker = gst_rtp_buffer_get_marker (buf); + if (marker && outbuf) { and here ::: gst/rtp/gstrtpg726depay.c @@ +343,3 @@ return outbuf; + +bad_len: and here... and in all other places
I decided not to print any error because the warning that I mentioned is always displayed when NULL is returned, but if you prefer I'll display an error.
I think there should be at least a GST_ERROR_OBJECT() when this happens, but maybe in the function that returns NULL instead of the buffer.
Sure I can add an error to the function that returns null, but it is a different module in this file: gst-plugins-base/gst-libs/gst/rtp/gstrtpbuffer.c the function is this one: gst_rtp_buffer_get_payload_subbuffer The warning is printed with g_warning (what I think is not recommended for GStreamer) Should I send another patch changing the g_warning by an GST_ERROR_OBJECT and file a new bug?
Oh sorry, I misread what you've written. No, the patch is fine as-is, I'll push it. Thanks :) commit 9d322436711d6769d7ec7b0917a7b038e3591d7a Author: Jose Antonio Santos Cadenas <santoscadenas@gmail.com> Date: Wed May 18 12:36:40 2011 +0200 rtp: Fix segmentation fault processing payload buffers This commit checks if the value returned by gst_rtp_buffer_get_payload_buffer and gst_rtp_buffer_get_payload_subbuffer is NULL before using it.
Shouldn't gst_rtp_buffer_validate() have been called first ?
Yes, I think that is a better option to call gst_rtp_buffer_validate instead of just checking if it is NULL. Sorry, I'm new with gstreamer and I did not know this funtion. I can send a new patch with his modification it's OK for you.
Yes, please do :)
gst_rtp_buffer_validate() is called before by the base class, so it apparently doesn't catch this kind of error. Indeed, in this case, the packet is valid but it simply doesn't contain any data (and then _get_payload_(sub)bufer simply return NULL)
I mean calling gst_rtp_buffer_validate just after the get_payload_(sub)buffer instead of just checking if is NULL. Is this a good option?
(In reply to comment #10) > I mean calling gst_rtp_buffer_validate just after the get_payload_(sub)buffer > instead of just checking if is NULL. Is this a good option? No, just checking for NULL is good IMO. I think the patch was OK.