Re: [Ccrtp-devel] Small bug in RTCP part when sending a BYE

[Werner Dittmann]

fix of the fix:

after cheking the padding rules again in RFC3550, chapter 6.6 the reason string
must be filled with null bytes until it fills to the next 32bit boundary. This
padding is different from padding indicated in the header using the P bit.

This filling of the string must be reflected in the length byte for the
string. Thus this is the correct fix (tested with wireshark):

...
        if ( reason.c_str() != NULL ){
                pkt->info.BYE.length = (uint8)strlen(reason.c_str());
                memcpy(buffer + len,reason.c_str(),pkt->info.BYE.length);
                len += pkt->info.BYE.length;
                padlen = 4 - ((len - len1) & 0x03);
                if ( padlen ) {
                        memset(buffer + len,0,padlen);
                        len += padlen;
                        pkt->info.BYE.length += padlen;  // take filler bytes 
into account
                }
        }
        pkt->fh.length = htons(((len - len1) >> 2) - 1);
        //pkt->fh.padding = (padlen > 0);
...

Regards,
Werner

Werner Dittmann schrieb:
> During several tests wireshark always reports a malformed RTCP BYE packet.
> 
> Looking at the problem shows that the BYE packet has the padding bit set
> but the padding length is zero. According to the specification the padding
> length should be the number of padding bytes including the length byte
> itself.
> 
> While the method that constructs the BYE packet correctly detects and computes
> the padding length it does not set the padding length itself in the buffer.
> 
> The offending code is in control.cpp, dispatchBYE(...), near the end:
> 
> ...
>       if ( reason.c_str() != NULL ){
>               pkt->info.BYE.length = (uint8)strlen(reason.c_str());
>               memcpy(buffer + len,reason.c_str(),pkt->info.BYE.length);
>               len += pkt->info.BYE.length;
>               padlen = 4 - ((len - len1) & 0x03);
>               if ( padlen ) {
>                       memset(buffer + len,0,padlen);
>                       len += padlen;
>               }
>       }
>       pkt->fh.length = htons(((len - len1) >> 2) - 1);
>       pkt->fh.padding = (padlen > 0);
> ...
> 
> In this case the padding length is 1 (depends on the reason string). As
> seen in the code snippet the padding bit is set (fh.padding) but the padding
> length itself is not. This should be the last byte in the buffer:
> 
> ...
>               if ( padlen ) {
>                       memset(buffer + len,0,padlen);
>                       len += padlen;
>                       *(buffer+len-1) = padlen
>               }
> ...
> 
> 
> Regards,
> Werner
> 
> 
> _______________________________________________
> Ccrtp-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/ccrtp-devel
>