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 1/7] 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); From 75b2ff0f1af3c8e9cce03d96748bf4be5b4d142d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Wed, 18 Oct 2023 19:47:12 +0200 Subject: [PATCH 2/7] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5f49e6839..5a4c4e7e96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### NEXT * CI: Use Node.js version 20 ([PR #1177](https://github.com/versatica/mediasoup/pull/1177)). +* Fix `IceServer` crash when client uses ICE nomination ([PR #1182](https://github.com/versatica/mediasoup/pull/1182)). ### 3.12.13 From 422001884d56a0b5f0bbc09f5d89374f5be661b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Wed, 18 Oct 2023 19:48:52 +0200 Subject: [PATCH 3/7] fix CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a4c4e7e96..f3dc27490b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ ### NEXT * CI: Use Node.js version 20 ([PR #1177](https://github.com/versatica/mediasoup/pull/1177)). -* Fix `IceServer` crash when client uses ICE nomination ([PR #1182](https://github.com/versatica/mediasoup/pull/1182)). +* Fix `IceServer` crash when client uses ICE renomination ([PR #1182](https://github.com/versatica/mediasoup/pull/1182)). ### 3.12.13 From 7115b45de7fa8acd109370b78075bd91f1137360 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 19 Oct 2023 10:26:20 +0200 Subject: [PATCH 4/7] cosmetic --- worker/include/RTC/IceServer.hpp | 8 +++++--- worker/src/RTC/IceServer.cpp | 19 +++++++++++++------ worker/src/RTC/WebRtcTransport.cpp | 14 ++++++++------ 3 files changed, 26 insertions(+), 15 deletions(-) diff --git a/worker/include/RTC/IceServer.hpp b/worker/include/RTC/IceServer.hpp index 3b45e3d49c..cd91938bee 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( diff --git a/worker/src/RTC/IceServer.cpp b/worker/src/RTC/IceServer.cpp index c627c21db9..e3e963788d 100644 --- a/worker/src/RTC/IceServer.cpp +++ b/worker/src/RTC/IceServer.cpp @@ -337,18 +337,25 @@ namespace RTC } } - 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); diff --git a/worker/src/RTC/WebRtcTransport.cpp b/worker/src/RTC/WebRtcTransport.cpp index b5a54fd986..70ef85dcca 100644 --- a/worker/src/RTC/WebRtcTransport.cpp +++ b/worker/src/RTC/WebRtcTransport.cpp @@ -1058,10 +1058,11 @@ namespace RTC } // Trick for clients performing aggressive ICE regardless we are ICE-Lite. - // NOTE: Only do it if we already have a selected ICE candidate tuple. - if (this->iceServer->GetSelectedTuple()) + if ( + this->iceServer->GetState() == RTC::IceServer::IceState::CONNECTED || + this->iceServer->GetState() == RTC::IceServer::IceState::COMPLETED) { - this->iceServer->ForceSelectedTuple(tuple); + this->iceServer->MayForceSelectedTuple(tuple); } // Check that DTLS status is 'connecting' or 'connected'. @@ -1146,10 +1147,11 @@ namespace RTC } // Trick for clients performing aggressive ICE regardless we are ICE-Lite. - // NOTE: Only do it if we already have a selected ICE candidate tuple. - if (this->iceServer->GetSelectedTuple()) + if ( + this->iceServer->GetState() == RTC::IceServer::IceState::CONNECTED || + this->iceServer->GetState() == RTC::IceServer::IceState::COMPLETED) { - this->iceServer->ForceSelectedTuple(tuple); + this->iceServer->MayForceSelectedTuple(tuple); } // Pass the packet to the parent transport. From 062277a3662d0d7a0f28f818952b6afccbf69839 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 19 Oct 2023 11:49:27 +0200 Subject: [PATCH 5/7] Update worker/include/RTC/IceServer.hpp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: José Luis Millán --- worker/include/RTC/IceServer.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/worker/include/RTC/IceServer.hpp b/worker/include/RTC/IceServer.hpp index cd91938bee..b213189ff5 100644 --- a/worker/include/RTC/IceServer.hpp +++ b/worker/include/RTC/IceServer.hpp @@ -94,7 +94,7 @@ 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 + * 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); From c975ac30e53d2859bde3df2296a4c9b2d8b32121 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 19 Oct 2023 11:50:27 +0200 Subject: [PATCH 6/7] Remvoe duplicate condition --- worker/src/RTC/WebRtcTransport.cpp | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/worker/src/RTC/WebRtcTransport.cpp b/worker/src/RTC/WebRtcTransport.cpp index 70ef85dcca..4846006957 100644 --- a/worker/src/RTC/WebRtcTransport.cpp +++ b/worker/src/RTC/WebRtcTransport.cpp @@ -1058,12 +1058,7 @@ namespace RTC } // Trick for clients performing aggressive ICE regardless we are ICE-Lite. - if ( - this->iceServer->GetState() == RTC::IceServer::IceState::CONNECTED || - this->iceServer->GetState() == RTC::IceServer::IceState::COMPLETED) - { - this->iceServer->MayForceSelectedTuple(tuple); - } + this->iceServer->MayForceSelectedTuple(tuple); // Check that DTLS status is 'connecting' or 'connected'. if ( @@ -1147,12 +1142,7 @@ namespace RTC } // Trick for clients performing aggressive ICE regardless we are ICE-Lite. - if ( - this->iceServer->GetState() == RTC::IceServer::IceState::CONNECTED || - this->iceServer->GetState() == RTC::IceServer::IceState::COMPLETED) - { - this->iceServer->MayForceSelectedTuple(tuple); - } + this->iceServer->MayForceSelectedTuple(tuple); // Pass the packet to the parent transport. RTC::Transport::ReceiveRtpPacket(packet); From 5cad037fe2837eb91df61f623357436eb2a24123 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 19 Oct 2023 12:27:12 +0200 Subject: [PATCH 7/7] Make AddTuple() internally check HasTuple() --- worker/src/RTC/IceServer.cpp | 61 +++++++++++++----------------------- 1 file changed, 22 insertions(+), 39 deletions(-) diff --git a/worker/src/RTC/IceServer.cpp b/worker/src/RTC/IceServer.cpp index e3e963788d..4cf1a6a924 100644 --- a/worker/src/RTC/IceServer.cpp +++ b/worker/src/RTC/IceServer.cpp @@ -397,13 +397,8 @@ namespace RTC } 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) { @@ -464,13 +459,8 @@ namespace RTC } 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) { @@ -512,11 +502,8 @@ namespace RTC if (!hasUseCandidate && !hasNomination) { - // If a new tuple store it. - if (!HasTuple(tuple)) - { - AddTuple(tuple); - } + // Store the tuple. + AddTuple(tuple); } else { @@ -528,13 +515,8 @@ 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) { @@ -568,21 +550,13 @@ 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) { @@ -606,10 +580,19 @@ 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).