diff --git a/CHANGELOG.md b/CHANGELOG.md index fa51a3c085..a9a9ff4a50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,11 @@ # Changelog +### Next + +* Fix `IceServer` crash when client uses ICE renomination ([PR #1182](https://github.com/versatica/mediasoup/pull/1182)). + + ### 3.12.15 * Fix NPM "postinstall" task in Windows ([PR #1187](https://github.com/versatica/mediasoup/pull/1187)). diff --git a/worker/include/RTC/IceServer.hpp b/worker/include/RTC/IceServer.hpp index 51fa85919d..b213189ff5 100644 --- a/worker/include/RTC/IceServer.hpp +++ b/worker/include/RTC/IceServer.hpp @@ -93,9 +93,11 @@ namespace RTC } bool IsValidTuple(const RTC::TransportTuple* tuple) const; void RemoveTuple(RTC::TransportTuple* tuple); - // This should be just called in 'connected' or completed' state - // and the given tuple must be an already valid tuple. - void ForceSelectedTuple(const RTC::TransportTuple* tuple); + /** + * This should be just called in 'connected' or 'completed' state and the + * given tuple must be an already valid tuple. + */ + void MayForceSelectedTuple(const RTC::TransportTuple* tuple); private: void HandleTuple( @@ -122,8 +124,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..4cf1a6a924 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,24 +327,35 @@ namespace RTC { // Update state. this->state = IceState::DISCONNECTED; + + // Reset remote nomination. + this->remoteNomination = 0u; + // Notify the listener. this->listener->OnIceServerDisconnected(this); } } } - void IceServer::ForceSelectedTuple(const RTC::TransportTuple* tuple) + void IceServer::MayForceSelectedTuple(const RTC::TransportTuple* tuple) { MS_TRACE(); - MS_ASSERT( - this->selectedTuple, "cannot force the selected tuple if there was not a selected tuple"); + if (this->state != IceState::CONNECTED && this->state != IceState::COMPLETED) + { + MS_WARN_TAG(ice, "cannot force selected tuple if not in state 'connected' or 'completed'"); + + return; + } auto* storedTuple = HasTuple(tuple); - MS_ASSERT( - storedTuple, - "cannot force the selected tuple if the given tuple was not already a valid tuple"); + if (!storedTuple) + { + MS_WARN_TAG(ice, "cannot force selected tuple if the given tuple was not already a valid one"); + + return; + } // Mark it as selected tuple. SetSelectedTuple(storedTuple); @@ -351,10 +370,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,8 +388,10 @@ namespace RTC // Mark it as selected tuple. SetSelectedTuple(storedTuple); + // Update state. this->state = IceState::CONNECTED; + // Notify the listener. this->listener->OnIceServerConnected(this); } @@ -395,11 +412,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 +432,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,8 +450,10 @@ namespace RTC // Mark it as selected tuple. SetSelectedTuple(storedTuple); + // Update state. this->state = IceState::CONNECTED; + // Notify the listener. this->listener->OnIceServerConnected(this); } @@ -456,11 +474,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); } @@ -479,9 +502,8 @@ namespace RTC if (!hasUseCandidate && !hasNomination) { - // If a new tuple store it. - if (!HasTuple(tuple)) - AddTuple(tuple); + // Store the tuple. + AddTuple(tuple); } else { @@ -493,21 +515,23 @@ namespace RTC hasNomination ? "true" : "false", nomination); - auto* storedTuple = HasTuple(tuple); - - // If a new tuple store it. - if (!storedTuple) - storedTuple = AddTuple(tuple); + // Store the tuple. + auto* 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); } @@ -526,25 +550,24 @@ namespace RTC if (!hasUseCandidate && !hasNomination) { - // If a new tuple store it. - if (!HasTuple(tuple)) - AddTuple(tuple); + // Store the tuple. + AddTuple(tuple); } else { - auto* storedTuple = HasTuple(tuple); - - // If a new tuple store it. - if (!storedTuple) - storedTuple = AddTuple(tuple); + // Store the tuple. + auto* 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; + } } } @@ -557,15 +580,26 @@ namespace RTC { MS_TRACE(); + auto* storedTuple = HasTuple(tuple); + + if (storedTuple) + { + MS_DEBUG_DEV('tuple already exists'); + + return storedTuple; + } + // Add the new tuple at the beginning of the list. this->tuples.push_front(*tuple); - auto* storedTuple = std::addressof(*this->tuples.begin()); + storedTuple = std::addressof(*this->tuples.begin()); // 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 +648,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 +660,9 @@ namespace RTC auto* storedTuple = const_cast(std::addressof(it)); if (storedTuple->Compare(tuple)) + { return storedTuple; + } } return nullptr; @@ -641,7 +674,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..4846006957 100644 --- a/worker/src/RTC/WebRtcTransport.cpp +++ b/worker/src/RTC/WebRtcTransport.cpp @@ -1058,7 +1058,7 @@ namespace RTC } // Trick for clients performing aggressive ICE regardless we are ICE-Lite. - this->iceServer->ForceSelectedTuple(tuple); + this->iceServer->MayForceSelectedTuple(tuple); // Check that DTLS status is 'connecting' or 'connected'. if ( @@ -1142,7 +1142,7 @@ namespace RTC } // Trick for clients performing aggressive ICE regardless we are ICE-Lite. - this->iceServer->ForceSelectedTuple(tuple); + this->iceServer->MayForceSelectedTuple(tuple); // Pass the packet to the parent transport. RTC::Transport::ReceiveRtpPacket(packet);