Fix RTCP SDES #1139

ibc · 2023-08-09T13:11:38Z

Details

Initially this PR was intended to remove a non legit warning log. A SDES chunk may perfecly terminate with a single null octet (AKA item type === 0) and, if so, we MUST NOT check the next byte (item length) because it may not exist (if chunk is padded to 4 bytes) and, if it exists, it should be one or more zeroes (up to 3) to pad the chunk to 4 bytes.
However then I've added more tests and replicated this one I've written in rtp.js library: https://github.com/versatica/rtp.js/blob/master/src/tests/rtcp/SdesPacket.test.ts#L195 and I also refactored the existing SDES test to test more things. And LOT of problems show up.

Bonus Tracks

Use Node 18 in Dockerfile.

Issue 2

UPDATE: Fixed in this commit: 1d107e0

Parsing of this SDES proper packet fails miserably ("parse packet 2" section in TestSdes.cpp in this branch):

0x82, 0xca, 0x00, 0x0c, // Type: 202 (SDES), Count: 2, Length: 12
// Chunk 2
0x00, 0x00, 0x04, 0xd2, // SSRC: 1234
0x01, 0x06, 0x71, 0x77, // Item Type: 1 (CNAME), Length: 6, Text: "qwerty"
0x65, 0x72, 0x74, 0x79,
0x06, 0x06, 0x69, 0xc3, // Item Type: 6 (TOOL), Length: 6, Text: "iñaki"
0xb1, 0x61, 0x6b, 0x69,
0x00, 0x00, 0x00, 0x00, // 4 null octets
// Chunk 3
0x00, 0x00, 0x16, 0x2e, // SSRC: 5678
0x05, 0x11, 0x73, 0x6f, // Item Type: 5 (LOC), Length: 17, Text: "somewhere œæ€"
0x6d, 0x65, 0x77, 0x68,
0x65, 0x72, 0x65, 0x20,
0xc5, 0x93, 0xc3, 0xa6,
0xe2, 0x82, 0xac, 0x00  // 1 null octet

packet->GetSize() should return 52 bytes however it returns 28 which is the byte length of the SDES header (4 bytes) plus only the first chunk (24 bytes), but there are 2 chunks!
However packet->GetCount() returns 2 as expected, and given that the code it runs is return this->chunks.size(); it clearly means that it's parsing the second chunk with length 0 (wrong, it's 24).

I'm pretty sure that the SDES parsing in rtp.js is way better than here. For instance, by checking Sdes.cpp code I don't see any evidence that it's properly processing chunk null octets.

As per spec, each SDES chunk MUST be padded to 4 bytes with null octets at the end.
In addition, each chunk MUST end with 1, 2, 3 or 4 mandatory null octets. Why? Because "item type === 0" is the only way to say "this is the end of this chunk", and the remaining 1, 2 or 3 null octets are needed in case of padding.

So in packet above, chunk 1 ends with 4 null octets (which are part of the chunk) because the 2 items it contains are already padded to 4 bytes, so we need to add a "item type 0" null octet to define "end of items in this chunk", so we also need to add another 3 null octets for padding.

To make it clear: DO NOT try to parse as many null octets as possible. We must only parse from 1 to 4 null octet and stop once the chunk is padded to 4 bytes. So for example, in the packet above, look at the second/last item item in the first chunk:

0x06, 0x06, 0x69, 0xc3, // Item Type: 6 (TOOL), Length: 6, Text: "iñaki"
0xb1, 0x61, 0x6b, 0x69,
0x00, 0x00, 0x00, 0x00, // 4 null octets

And then look at the SSRC line of the second chunk:

0x00, 0x00, 0x16, 0x2e, // SSRC: 5678

Yes! it starts with 2 zeroes, but this is because SSRC is 5678 so 0x00, 0x00, 0x16, 0x2e!

I'm pretty sure that parsing in mediasoup Sdes.cpp is not considering this at all.

Check the SDES parser of rtp.js to see how it does it properly: https://github.com/versatica/rtp.js/blob/master/src/rtcp/SdesPacket.ts?ts=2#L111
And also the constructor of SdesChunk: https://github.com/versatica/rtp.js/blob/master/src/rtcp/SdesPacket.ts#L336

Issue 3

UPDATE: Fixed in this commit: 1114f97

I've added "parse packet 3" e303b74 and the test fails since it cannot parse packet 3.

*NOTE:* WIP due to the so many bugs discovered and not yet fixed. ### Details - Initially this PR was intended to remove a non legit warning log. A SDES chunk may perfecly terminate with a single null octet (AKA item type === 0) and, if so, we MUST NOT check the next byte because it may not exist (if chunk is padded to 4 bytes) and, if it exists, it should be one or more zeroes (up to 3) to pad the chunk to 4 bytes. So this is done. - However then I wanted to add more tests and replicated this one I've written in rtp.js library: https://github.com/versatica/rtp.js/blob/master/src/tests/rtcp/SdesPacket.test.ts#L195 and I also refactored the existing SDES test to test more things. And LOT of problems show up. ### Issue 1 If you run the worker tests, it enters into an infinite loop. This is because these loops in `TestSdes.cpp` **never** exist: ```c++ for (auto* chunk = *(packet->Begin()); chunk != *(packet->End()); ++chunk, ++chunkIdx) ``` ```c++ for (auto* item = *(chunk->Begin()); item != *(chunk->End()); ++item, ++itemIdx) ``` It prints this: ``` RTC::RTCP::TestSdes::____C_A_T_C_H____T_E_S_T____0() | ---- parse packet 1 | chunkIdx:0, chunk address:105553180051264 RTC::RTCP::TestSdes::____C_A_T_C_H____T_E_S_T____0() | ---- parse packet 1 | itemIdx:0, item address:105553182076928 RTC::RTCP::TestSdes::____C_A_T_C_H____T_E_S_T____0() | ---- parse packet 1 | itemIdx:1, item address:105553182076944 RTC::RTCP::TestSdes::____C_A_T_C_H____T_E_S_T____0() | ---- parse packet 1 | itemIdx:2, item address:105553182076960 RTC::RTCP::TestSdes::____C_A_T_C_H____T_E_S_T____0() | ---- parse packet 1 | itemIdx:3, item address:105553182076976 RTC::RTCP::TestSdes::____C_A_T_C_H____T_E_S_T____0() | ---- parse packet 1 | itemIdx:4, item address:105553182076992 RTC::RTCP::TestSdes::____C_A_T_C_H____T_E_S_T____0() | ---- parse packet 1 | itemIdx:5, item address:105553182077008 RTC::RTCP::TestSdes::____C_A_T_C_H____T_E_S_T____0() | ---- parse packet 1 | itemIdx:6, item address:105553182077024 RTC::RTCP::TestSdes::____C_A_T_C_H____T_E_S_T____0() | ---- parse packet 1 | itemIdx:7, item address:105553182077040 RTC::RTCP::TestSdes::____C_A_T_C_H____T_E_S_T____0() | ---- parse packet 1 | itemIdx:8, item address:105553182077056 RTC::RTCP::TestSdes::____C_A_T_C_H____T_E_S_T____0() | ---- parse packet 1 | itemIdx:9, item address:105553182077072 RTC::RTCP::TestSdes::____C_A_T_C_H____T_E_S_T____0() | ---- parse packet 1 | itemIdx:10, item address:105553182077088 ``` For me it's like if `++item` (same for `++chunk`) doesn't move the iterator to real next vector position so it never matches `End()`. As far as I see, the whole mediasoup worker code doesn't rely much on this `Begin()` and `End()` API which explains that we may have never noticed this bug. ### Issue 2 Parsing of this SDES proper packet fails miserably ("parse packet 2" section in `TestSdes.cpp`): ``` 0x82, 0xca, 0x00, 0x0c, // Type: 202 (SDES), Count: 2, Length: 12 // Chunk 2 0x00, 0x00, 0x04, 0xd2, // SSRC: 1234 0x01, 0x06, 0x71, 0x77, // Item Type: 1 (CNAME), Length: 6, Text: "qwerty" 0x65, 0x72, 0x74, 0x79, 0x06, 0x06, 0x69, 0xc3, // Item Type: 6 (TOOL), Length: 6, Text: "iñaki" 0xb1, 0x61, 0x6b, 0x69, 0x00, 0x00, 0x00, 0x00, // 4 null octects // Chunk 3 0x00, 0x00, 0x16, 0x2e, // SSRC: 5678 0x05, 0x11, 0x73, 0x6f, // Item Type: 5 (LOC), Length: 17, Text: "somewhere œæ€" 0x6d, 0x65, 0x77, 0x68, 0x65, 0x72, 0x65, 0x20, 0xc5, 0x93, 0xc3, 0xa6, 0xe2, 0x82, 0xac, 0x00 // 1 null octect ``` 1. `packet->GetSize()` should return 52 bytes however it returns 28 which is the byte length of the SDES header (4 bytes) plus only the first chunk (24 bytes), but there are 2 chunks!. // REQUIRE(packet->GetSize() == 52); 2. However `packet->GetCount()` returns 2 as expected, and given that the code it runs is `return this->chunks.size();` it clearly means that it's parsing the second chunk with length 0 (wrong, it's 24). I'm pretty sure that the SDES parsing in rtp.js is way better than here. For instance, by checking `Sdes.cpp` code I don't see any evidence that it's properly processing chunk null octects. * As per spec, each SDES chunk MUST be padded to 4 bytes with null octects at the end. * In addition, each chunk MUST end with 1, 2, 3 or 4 mandatory null octects. Why? Because "item type === 0" is the only way to say "this is the end of this chunk", and the remaining 1, 2 or 3 null octects are needed in case of padding. So in packet above, chunk 1 ends with 4 null octects (which are part of the chunk) because the 2 items it contains are already padded to 4 bytes, so we need to add a "item type 0" null octect to define "end of items in this chunk", so we also need to add another 3 null octects for padding. However I'm pretty sure that parsing in mediasoup `Sdes.cpp` is not considering this at all. Check the SDES parser of rtp.js to see how it does it properly: https://github.com/versatica/rtp.js/blob/master/src/rtcp/SdesPacket.ts?ts=2#L111

ibc · 2023-08-09T16:09:41Z

I have fixed issue 2 in this commit (but tests are missing until issue 1 is fixed):

1d107e0

ibc · 2023-08-09T16:10:09Z

And issue 1 is not a bug because I am stupid. Properly using vector iterator now. Will push soon.

ibc · 2023-08-09T16:58:17Z

I've fixed Issues 1 and 2, but have found a new issue 3. See description above.

ibc · 2023-08-09T17:46:32Z

Everything is fixed and tested. Running fuzzer before merging.

ibc · 2023-08-09T18:21:32Z

We are good.

ibc added the bug label Aug 9, 2023

ibc added this to the v3 updates milestone Aug 9, 2023

ibc requested a review from jmillan August 9, 2023 13:11

fix typo

63ae44a

jmillan approved these changes Aug 9, 2023

View reviewed changes

ibc added 2 commits August 9, 2023 17:42

Fix mandatory null octets not being considered in SDES chunks

1d107e0

comment infinite loop to avoid GitHub C to ban us

dc22fec

ibc added 2 commits August 9, 2023 18:36

yes!

9a046ca

Adding a crashing test (packet 3) :(

e303b74

fixed!!!

1114f97

ibc marked this pull request as ready for review August 9, 2023 17:46

ibc added 2 commits August 9, 2023 19:48

make format

6a40716

Use Node 18 in Dockerfile

e9418bc

ibc merged commit 6697804 into v3 Aug 9, 2023
34 checks passed

ibc deleted the fix-rtcp-sdes branch August 9, 2023 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RTCP SDES #1139

Fix RTCP SDES #1139

ibc commented Aug 9, 2023 •

edited

ibc commented Aug 9, 2023

ibc commented Aug 9, 2023

ibc commented Aug 9, 2023

ibc commented Aug 9, 2023

ibc commented Aug 9, 2023

Fix RTCP SDES #1139

Fix RTCP SDES #1139

Conversation

ibc commented Aug 9, 2023 • edited

Details

Bonus Tracks

Issue 2

Issue 3

ibc commented Aug 9, 2023

ibc commented Aug 9, 2023

ibc commented Aug 9, 2023

ibc commented Aug 9, 2023

ibc commented Aug 9, 2023

ibc commented Aug 9, 2023 •

edited