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 650470 - RTP: Some depayloaders break with malformed rtp packets
RTP: Some depayloaders break with malformed rtp packets
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal major
: 0.10.30
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2011-05-18 11:30 UTC by Jose Antonio Santos Cadenas
Modified: 2011-05-19 08:29 UTC
See Also:
GNOME target: ---
GNOME version: ---

Patch that solves the described problem (8.43 KB, patch)
2011-05-18 11:30 UTC, Jose Antonio Santos Cadenas
committed Details | Review

Description Jose Antonio Santos Cadenas 2011-05-18 11:30:10 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.
Comment 1 Sebastian Dröge (slomo) 2011-05-18 12:04:55 UTC
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;

and here... and in all other places
Comment 2 Jose Antonio Santos Cadenas 2011-05-18 12:09:07 UTC
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.
Comment 3 Sebastian Dröge (slomo) 2011-05-18 12:18:51 UTC
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.
Comment 4 Jose Antonio Santos Cadenas 2011-05-18 12:30:43 UTC
Sure I can add an error to the function that returns null, but it is a different module in this file:


the function is this one:


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?
Comment 5 Sebastian Dröge (slomo) 2011-05-18 13:25:48 UTC
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 <>
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.
Comment 6 Olivier Crête 2011-05-18 14:37:45 UTC
Shouldn't gst_rtp_buffer_validate() have been called first ?
Comment 7 Jose Antonio Santos Cadenas 2011-05-19 07:48:58 UTC
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.
Comment 8 Sebastian Dröge (slomo) 2011-05-19 08:00:06 UTC
Yes, please do :)
Comment 9 Wim Taymans 2011-05-19 08:00:38 UTC
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)
Comment 10 Jose Antonio Santos Cadenas 2011-05-19 08:26:52 UTC
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?
Comment 11 Wim Taymans 2011-05-19 08:29:47 UTC
(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.