From dad601470e53b24ae26d767290b56173009f755b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Wed, 18 Oct 2023 19:45:30 +0200 Subject: [PATCH] Fix IceServer crash when client uses ICE nomination Fixes #1176 ### Details - Problem was in this PR 756 that added support for ICE nomination. - Before that PR, we always assumed that if there is a tuple then we are in 'connected' or 'completed' state, so there is a selected ICE tuple. - That's no longer true when client uses ICE nomination stuff. --- worker/include/RTC/IceServer.hpp | 2 +- worker/src/RTC/IceServer.cpp | 87 ++++++++++++++++++++++-------- worker/src/RTC/WebRtcTransport.cpp | 12 ++++- 3 files changed, 77 insertions(+), 24 deletions(-) diff --git a/worker/include/RTC/IceServer.hpp b/worker/include/RTC/IceServer.hpp index 51fa85919d..3b45e3d49c 100644 --- a/worker/include/RTC/IceServer.hpp +++ b/worker/include/RTC/IceServer.hpp @@ -122,8 +122,8 @@ namespace RTC std::string password; std::string oldUsernameFragment; std::string oldPassword; - uint32_t remoteNomination{ 0u }; IceState state{ IceState::NEW }; + uint32_t remoteNomination{ 0u }; std::list tuples; RTC::TransportTuple* selectedTuple{ nullptr }; }; diff --git a/worker/src/RTC/IceServer.cpp b/worker/src/RTC/IceServer.cpp index fde3d25a1b..c627c21db9 100644 --- a/worker/src/RTC/IceServer.cpp +++ b/worker/src/RTC/IceServer.cpp @@ -220,9 +220,13 @@ namespace RTC // Authenticate the response. if (this->oldPassword.empty()) + { response->Authenticate(this->password); + } else + { response->Authenticate(this->oldPassword); + } // Send back. response->Serialize(StunSerializeBuffer); @@ -233,7 +237,9 @@ namespace RTC uint32_t nomination{ 0u }; if (packet->HasNomination()) + { nomination = packet->GetNomination(); + } // Handle the tuple. HandleTuple(tuple, packet->HasUseCandidate(), packet->HasNomination(), nomination); @@ -294,7 +300,9 @@ namespace RTC // If not found, ignore. if (!removedTuple) + { return; + } // Notify the listener. this->listener->OnIceServerTupleRemoved(this, removedTuple); @@ -319,6 +327,10 @@ namespace RTC { // Update state. this->state = IceState::DISCONNECTED; + + // Reset remote nomination. + this->remoteNomination = 0u; + // Notify the listener. this->listener->OnIceServerDisconnected(this); } @@ -351,10 +363,6 @@ namespace RTC { case IceState::NEW: { - // There should be no tuples. - MS_ASSERT( - this->tuples.empty(), "state is 'new' but there are %zu tuples", this->tuples.size()); - // There shouldn't be a selected tuple. MS_ASSERT(!this->selectedTuple, "state is 'new' but there is selected tuple"); @@ -373,15 +381,22 @@ namespace RTC // Mark it as selected tuple. SetSelectedTuple(storedTuple); + // Update state. this->state = IceState::CONNECTED; + // Notify the listener. this->listener->OnIceServerConnected(this); } else { - // Store the tuple. - auto* storedTuple = AddTuple(tuple); + auto* storedTuple = HasTuple(tuple); + + // If a new tuple store it. + if (!storedTuple) + { + storedTuple = AddTuple(tuple); + } if ((hasNomination && nomination > this->remoteNomination) || !hasNomination) { @@ -395,11 +410,16 @@ namespace RTC // Mark it as selected tuple. SetSelectedTuple(storedTuple); + // Update state. this->state = IceState::COMPLETED; + // Update nomination. if (hasNomination && nomination > this->remoteNomination) + { this->remoteNomination = nomination; + } + // Notify the listener. this->listener->OnIceServerCompleted(this); } @@ -410,12 +430,6 @@ namespace RTC case IceState::DISCONNECTED: { - // There should be no tuples. - MS_ASSERT( - this->tuples.empty(), - "state is 'disconnected' but there are %zu tuples", - this->tuples.size()); - // There shouldn't be a selected tuple. MS_ASSERT(!this->selectedTuple, "state is 'disconnected' but there is selected tuple"); @@ -434,15 +448,22 @@ namespace RTC // Mark it as selected tuple. SetSelectedTuple(storedTuple); + // Update state. this->state = IceState::CONNECTED; + // Notify the listener. this->listener->OnIceServerConnected(this); } else { - // Store the tuple. - auto* storedTuple = AddTuple(tuple); + auto* storedTuple = HasTuple(tuple); + + // If a new tuple store it. + if (!storedTuple) + { + storedTuple = AddTuple(tuple); + } if ((hasNomination && nomination > this->remoteNomination) || !hasNomination) { @@ -456,11 +477,16 @@ namespace RTC // Mark it as selected tuple. SetSelectedTuple(storedTuple); + // Update state. this->state = IceState::COMPLETED; + // Update nomination. if (hasNomination && nomination > this->remoteNomination) + { this->remoteNomination = nomination; + } + // Notify the listener. this->listener->OnIceServerCompleted(this); } @@ -481,7 +507,9 @@ namespace RTC { // If a new tuple store it. if (!HasTuple(tuple)) + { AddTuple(tuple); + } } else { @@ -497,17 +525,24 @@ namespace RTC // If a new tuple store it. if (!storedTuple) + { storedTuple = AddTuple(tuple); + } if ((hasNomination && nomination > this->remoteNomination) || !hasNomination) { // Mark it as selected tuple. SetSelectedTuple(storedTuple); + // Update state. this->state = IceState::COMPLETED; + // Update nomination. if (hasNomination && nomination > this->remoteNomination) + { this->remoteNomination = nomination; + } + // Notify the listener. this->listener->OnIceServerCompleted(this); } @@ -528,7 +563,9 @@ namespace RTC { // If a new tuple store it. if (!HasTuple(tuple)) + { AddTuple(tuple); + } } else { @@ -536,15 +573,20 @@ namespace RTC // If a new tuple store it. if (!storedTuple) + { storedTuple = AddTuple(tuple); + } if ((hasNomination && nomination > this->remoteNomination) || !hasNomination) { // Mark it as selected tuple. SetSelectedTuple(storedTuple); + // Update nomination. if (hasNomination && nomination > this->remoteNomination) + { this->remoteNomination = nomination; + } } } @@ -565,7 +607,9 @@ namespace RTC // If it is UDP then we must store the remote address (until now it is // just a pointer that will be freed soon). if (storedTuple->GetProtocol() == TransportTuple::Protocol::UDP) + { storedTuple->StoreUdpRemoteAddress(); + } // Notify the listener. this->listener->OnIceServerTupleAdded(this, storedTuple); @@ -614,14 +658,11 @@ namespace RTC { MS_TRACE(); - // If there is no selected tuple yet then we know that the tuples list - // is empty. - if (!this->selectedTuple) - return nullptr; - - // Check the current selected tuple. - if (this->selectedTuple->Compare(tuple)) + // Check the current selected tuple (if any). + if (this->selectedTuple && this->selectedTuple->Compare(tuple)) + { return this->selectedTuple; + } // Otherwise check other stored tuples. for (const auto& it : this->tuples) @@ -629,7 +670,9 @@ namespace RTC auto* storedTuple = const_cast(std::addressof(it)); if (storedTuple->Compare(tuple)) + { return storedTuple; + } } return nullptr; @@ -641,7 +684,9 @@ namespace RTC // If already the selected tuple do nothing. if (storedTuple == this->selectedTuple) + { return; + } this->selectedTuple = storedTuple; diff --git a/worker/src/RTC/WebRtcTransport.cpp b/worker/src/RTC/WebRtcTransport.cpp index 000b389dbb..b5a54fd986 100644 --- a/worker/src/RTC/WebRtcTransport.cpp +++ b/worker/src/RTC/WebRtcTransport.cpp @@ -1058,7 +1058,11 @@ namespace RTC } // Trick for clients performing aggressive ICE regardless we are ICE-Lite. - this->iceServer->ForceSelectedTuple(tuple); + // NOTE: Only do it if we already have a selected ICE candidate tuple. + if (this->iceServer->GetSelectedTuple()) + { + this->iceServer->ForceSelectedTuple(tuple); + } // Check that DTLS status is 'connecting' or 'connected'. if ( @@ -1142,7 +1146,11 @@ namespace RTC } // Trick for clients performing aggressive ICE regardless we are ICE-Lite. - this->iceServer->ForceSelectedTuple(tuple); + // NOTE: Only do it if we already have a selected ICE candidate tuple. + if (this->iceServer->GetSelectedTuple()) + { + this->iceServer->ForceSelectedTuple(tuple); + } // Pass the packet to the parent transport. RTC::Transport::ReceiveRtpPacket(packet);