Ensure all INIT have a verification tag of 0 as per the spec #341
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #341 +/- ##
==========================================
- Coverage 81.32% 81.26% -0.06%
==========================================
Files 51 51
Lines 3336 3336
==========================================
- Hits 2713 2711 -2
- Misses 479 480 +1
- Partials 144 145 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@jmelancongen Is this Pion on both sides when you see this happen? I would like to create a reproduce (I can help with that!) Thank you so much for sending this PR over! |
|
I agree a reproducer or test would be nice. In the scenario where we are seeing this it is Pion on both sides so it should be doable. The specific test we are doing is a validation of our TURN provider, and I'm guessing the fact that there is a real network is important, I'm suspecting a specific packet being lost at the wrong moment during the initialization. I was lucky to find the webrtc-rs PR early so I haven't looked too deeply in the entire interaction, but we'll assemble the complete logs from the test and post them here and take a deeper look into it! |
|
Here is the SCTP handshake part of our test: sctp-handshake-hang.txt From what I was told, the test runs every minute in multiple locations, and it reproduces the issue once every 2-3 weeks globally. I've boiled down the logs to these bits: I believe the specific problem stems from:
I'm not too sure how to isolate those conditions in a smaller self-contained reproducer though. Since it feels like it needs the relay to be triggered. I'm also not very familiar with the testing framework for pion sadly, but it might be easier to reproduce this way? |
|
Fantastic! I can write a test for that today, that will be pretty easy to reproduce. |
dbd061d
to
9ee04dd
Compare
|
Sorry for the ping but did you have time to check on that? I'd would love to have the fix so we can remove a workaround on our side |
RFC 4960 Section 8.5.1 specifies that a packet containing an INIT chunk MUST set the verificate tag of the packet to 0. The current code follows that only if it has not received and handled an INIT packet from the peer yet. In which case the peerVerificationTag will not be zero, and any subsequent INIT will be invalid and refused by the peer. This is a problem if the INIT ACK was lost. If both peers have this behavior, and both INIT ACKs were lost, we fall into a situation where the initialization will never complete. Resolve this scenario by ensuring the packet with the INIT chunk always set the tag to zero, as is required.
9ee04dd
to
5472f31
Compare
Description
We've been seeing issues with one of our tests that sometime hangs while opening a DataChannel between two peers. Looking at the detailed logs we've been seeing this pattern:
and the data channel never opens
We've found out that there is a PR from a year ago in webrtc-rs that seems to fit exactly the issue we are hitting:
webrtc-rs/webrtc#405
Where the fix was to ensure the init packet always uses the 0 tag, instead of using the Association state which might be different? We're just bringing that same fix here.