Fix IceServer crash when client uses ICE renomination

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 renomination ([PR #1182](https://github.com/versatica/mediasoup/pull/1182)).
 
 
 ### 3.12.13

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<RTC::TransportTuple> tuples;
 		RTC::TransportTuple* selectedTuple{ nullptr };
 	};

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,15 +388,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 +417,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 +437,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 +455,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 +484,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 +514,9 @@ namespace RTC
 				{
 					// If a new tuple store it.
 					if (!HasTuple(tuple))
+					{
 						AddTuple(tuple);
+					}
 				}
 				else
 				{
@@ -497,17 +532,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,23 +570,30 @@ namespace RTC
 				{
 					// If a new tuple store it.
 					if (!HasTuple(tuple))
+					{
 						AddTuple(tuple);
+					}
 				}
 				else
 				{
 					auto* storedTuple = HasTuple(tuple);
 
 					// 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 +614,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,22 +665,21 @@ 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)
 		{
 			auto* storedTuple = const_cast<RTC::TransportTuple*>(std::addressof(it));
 
 			if (storedTuple->Compare(tuple))
+			{
 				return storedTuple;
+			}
 		}
 
 		return nullptr;
@@ -641,7 +691,9 @@ namespace RTC
 
 		// If already the selected tuple do nothing.
 		if (storedTuple == this->selectedTuple)
+		{
 			return;
+		}
 
 		this->selectedTuple = storedTuple;
 

worker/src/RTC/WebRtcTransport.cpp

@@ -1058,7 +1058,12 @@ namespace RTC
 		}
 
 		// Trick for clients performing aggressive ICE regardless we are ICE-Lite.
-		this->iceServer->ForceSelectedTuple(tuple);
+		if (
+		  this->iceServer->GetState() == RTC::IceServer::IceState::CONNECTED ||
+		  this->iceServer->GetState() == RTC::IceServer::IceState::COMPLETED)
+		{
+			this->iceServer->MayForceSelectedTuple(tuple);
+		}
 
 		// Check that DTLS status is 'connecting' or 'connected'.
 		if (
@@ -1142,7 +1147,12 @@ namespace RTC
 		}
 
 		// Trick for clients performing aggressive ICE regardless we are ICE-Lite.
-		this->iceServer->ForceSelectedTuple(tuple);
+		if (
+		  this->iceServer->GetState() == RTC::IceServer::IceState::CONNECTED ||
+		  this->iceServer->GetState() == RTC::IceServer::IceState::COMPLETED)
+		{
+			this->iceServer->MayForceSelectedTuple(tuple);
+		}
 
 		// Pass the packet to the parent transport.
 		RTC::Transport::ReceiveRtpPacket(packet);