Repository navigation
Conversation
STUNRequestCallback_AllocateRelay walks to the next TURN server once the current one times out, but the realm, nonce and key left over from the server that issued the 401 challenge are kept. QueueTURNRequest only skips authentication while m_strTURNRealm is empty, so the Allocate sent to the next server carries the previous server's realm and key. That server answers 401, the challenge handler ignores it because m_strTURNRealm is no longer empty, and the relay is marked failed. A second TURN server therefore only helps when the first one goes silent, not when it challenges and then goes away. Drop the cached realm and nonce when moving on to the next server so that it gets to issue its own challenge. The key does not need clearing: it is only used while m_strTURNRealm is set, and it is recomputed from whatever realm the next server sends. Reported in ValveSoftware#445.
|
Verified this change builds cleanly on Linux x86_64 with GCC 16 and Ninja without warnings or regressions. The class invariant |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #445.
The report is accurate, and the state that goes stale is the pair
m_strTURNRealm/m_strTURNNonce(and the key derived from them) on the ICE session interface. Every step of the failure is insteamnetworkingsockets_ice_client.cpp::2068-:2071:2091-:2103QueueTURNRequestonly skips authentication while the realm is empty:1032, then:1067-:1076:2045:2086-:2087So the request to the second server has the right username and the first server's realm and key, and its challenge is read as a wrong-credentials answer. Configuring a backup only survives the case where the primary never answers, which is what the report describes.
Change
This is the fix tintinhamans reports testing.
m_arrTURNKeyis deliberately left alone:QueueTURNRequestreturns before touching it wheneverm_strTURNRealmis empty (:1032), and it is rewritten from the new realm in the challenge handler (:2071), so it cannot be used once the realm is cleared. Clearing it would needmemsetin a function that does not currently include<string.h>.The 401 retry path (
:2078) re-queues to the same server immediately after setting that state, so it must and does keep it.Not in this change
Two things from the report and the surrounding code that are separate decisions, so I left them out:
Think_DiscoverRelayCandidate(:2000) always restarts from the first TURN server, and it is also the re-allocation path after a refresh timeout (:2140-:2143). If an allocation had previously failed over to the second server, that path re-uses the same stale realm and nonce against the first one. Same stale state, different entry point; the fix above does not cover it.Verification
Source level, not execution level: I followed the state through the five sites above and each step matches the report, and tintinhamans states that clearing these two members in this function fixed the failure for them.
I could not build or run this here.
tests/stun_server.pydoes implement TURN long-term credentials with a realm and nonce, andtest_p2p.pycan already give a client its own TURN server, so this is testable end to end: start two servers with different credentials where the first answers the challenge and then drops the authenticated Allocate, and assert that the relay still comes up through the second. That needs the GNS test binaries built, which I do not have in this environment, so I did not attempt it rather than guess at a test I cannot run.