GNOME Bugzilla – Bug 751883
rtcpbuffer: Fix validation of packets with padding
Last modified: 2015-08-16 13:40:35 UTC
Created attachment 306687 [details] [review] Patch with fix and tests Attached patch fixes the validation of rtcp buffers with padding. It also adds a few tests for both compound and reduzed size rtcp packets. The padding (if any) is included in the length of the last packet. Extracts from RFC 3550: Section 6.4.1: padding (P): 1 bit If the padding bit is set, this individual RTCP packet contains some additional padding octets at the end which are not part of the control information but are included in the length field. Section A.2: * The padding bit (P) should be zero for the first packet of a compound RTCP packet because padding should only be applied, if it is needed, to the last packet. * The length fields of the individual RTCP packets must add up to the overall length of the compound RTCP packet as received.
Review of attachment 306687 [details] [review]: ::: gst-libs/gst/rtp/gstrtcpbuffer.c @@ +150,2 @@ /* get padding */ + pad_bytes = data[-1]; Don't use negative array indices please @@ +150,3 @@ /* get padding */ + pad_bytes = data[-1]; + if (pad_bytes == 0 || pad_bytes >= header_len) The value of the padding should be the length of the packet? Or ...? ::: tests/check/libs/rtp.c @@ +824,3 @@ + + fail_unless (gst_rtcp_buffer_validate_data (rtcp_pkt, + sizeof (rtcp_pkt)) == TRUE); Don't compare to TRUE or FALSE, just do fail_if() or fail_unless()
Created attachment 306786 [details] [review] Patch with fix and tests Changed previous patch according to code review and also changed the wrong_padding check. The commit message includes a description of what the value of the so called pad byte should be. It's not the length of the packet as the previous code assumed, but it's the number of padded bytes at the end of the packet (including itself). I also changed the check for a valid pad byte value to be more in line what is explicitly stated in the RFC. It must be a multiple of 4, and the value 0 cannot be allowed since the count includes the pad byte itself. (Previous patch only check the upper and lower limit that made sense, but I realize that was a bit confusing).
commit 1586981b1bf88016ae2e39bde18e111306fbf7f2 Author: Stian Selnes <stian@pexip.com> Date: Thu Jul 2 20:50:00 2015 +0200 rtcpbuffer: Fix validation of packets with padding The padding (if any) is included in the length of the last packet, see RFC 3550. Section 6.4.1: padding (P): 1 bit If the padding bit is set, this individual RTCP packet contains some additional padding octets at the end which are not part of the control information but are included in the length field. The last octet of the padding is a count of how many padding octets should be ignored, including itself (it will be a multiple of four). Section A.2: * The padding bit (P) should be zero for the first packet of a compound RTCP packet because padding should only be applied, if it is needed, to the last packet. * The length fields of the individual RTCP packets must add up to the overall length of the compound RTCP packet as received. https://bugzilla.gnome.org/show_bug.cgi?id=751883