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 795159 - Various patches on ssrcdemux
Various patches on ssrcdemux
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-04-11 14:21 UTC by Håvard Graff (hgr)
Modified: 2018-11-03 15:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
rtpssrcdemux: introduce max-streams property (10.12 KB, patch)
2018-04-11 14:21 UTC, Håvard Graff (hgr)
needs-work Details | Review
rtpssrcdemux: Bad RTP/RTCP packet is not fatal (3.22 KB, patch)
2018-04-11 14:22 UTC, Håvard Graff (hgr)
accepted-commit_now Details | Review
rtpssrcdemux: Handle RTCP APP packets (3.15 KB, patch)
2018-04-11 14:23 UTC, Håvard Graff (hgr)
accepted-commit_now Details | Review
rtpssrcdemux: refactoring (8.23 KB, patch)
2018-04-11 14:24 UTC, Håvard Graff (hgr)
accepted-commit_now Details | Review

Description Håvard Graff (hgr) 2018-04-11 14:21:56 UTC
Created attachment 370796 [details] [review]
rtpssrcdemux: introduce max-streams property

The property is useful against attacks when the sender changes SSRC for
    every RTP packet. The property with the same name introduced in rtpbin
    was not enough, because we still can end up with thousands of pads
    allocated in rtpssrcdemux.
Comment 1 Håvard Graff (hgr) 2018-04-11 14:22:50 UTC
Created attachment 370797 [details] [review]
rtpssrcdemux: Bad RTP/RTCP packet is not fatal

When used for processing bundled media streams within rtpbin the rtpssrcdemux element may receive bad RTP and RTCP packets, these should not be treated as a fatal error.
Comment 2 Håvard Graff (hgr) 2018-04-11 14:23:30 UTC
Created attachment 370798 [details] [review]
rtpssrcdemux: Handle RTCP APP packets

Fix crash when processing RTCP APP packets.
Comment 3 Håvard Graff (hgr) 2018-04-11 14:24:05 UTC
Created attachment 370799 [details] [review]
rtpssrcdemux: refactoring

Avoid code-duplication and rename the lock to not appear as a native lock.
Comment 4 Olivier Crête 2018-04-17 15:46:28 UTC
Review of attachment 370796 [details] [review]:

::: gst/rtpmanager/gstrtpssrcdemux.c
@@ +284,3 @@
   }
+  // We create 2 src pads per ssrc (RTP & RTCP). Checking if we are allowed
+  // to create 2 more pads

Use /* */ style comments please

@@ +285,3 @@
+  // We create 2 src pads per ssrc (RTP & RTCP). Checking if we are allowed
+  // to create 2 more pads
+  num_streams = (GST_ELEMENT_CAST (demux)->numsrcpads) >> 1;

Why ">>1" ? Do you want to do / 2 really ?
Comment 5 Olivier Crête 2018-04-17 15:46:55 UTC
Review of attachment 370797 [details] [review]:

Looks good
Comment 6 Olivier Crête 2018-04-17 15:47:37 UTC
Review of attachment 370798 [details] [review]:

Looks good. Shouldn't there be a default: case that doesn't crash too?
Comment 7 Olivier Crête 2018-04-17 15:51:53 UTC
Review of attachment 370799 [details] [review]:

Looks ok
Comment 8 Håvard Graff (hgr) 2018-04-17 15:54:47 UTC
(In reply to Olivier Crête from comment #4)
> Review of attachment 370796 [details] [review] [review]:
> 
> ::: gst/rtpmanager/gstrtpssrcdemux.c
> @@ +284,3 @@
>    }
> +  // We create 2 src pads per ssrc (RTP & RTCP). Checking if we are allowed
> +  // to create 2 more pads
> 
> Use /* */ style comments please
> 
> @@ +285,3 @@
> +  // We create 2 src pads per ssrc (RTP & RTCP). Checking if we are allowed
> +  // to create 2 more pads
> +  num_streams = (GST_ELEMENT_CAST (demux)->numsrcpads) >> 1;
> 
> Why ">>1" ? Do you want to do / 2 really ?

Just that we get 2 pads for each stream, so this is a simple way of counting the number of streams I guess?
Comment 9 Håvard Graff (hgr) 2018-09-17 11:30:02 UTC
Ping?
Comment 10 Olivier Crête 2018-10-10 19:22:25 UTC
Arg, the patches no longer apply :-(
Comment 11 GStreamer system administrator 2018-11-03 15:28:48 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/461.