Skip to content

Commit

Permalink
Fix IceServer crash when client uses ICE nomination
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@ibc
ibc committed Oct 18, 2023
1 parent ef1b6b0 commit dad6014
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 24 deletions.
2 changes: 1 addition & 1 deletion worker/include/RTC/IceServer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<RTC::TransportTuple> tuples;
RTC::TransportTuple* selectedTuple{ nullptr };
};
Expand Down
87 changes: 66 additions & 21 deletions worker/src/RTC/IceServer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -294,7 +300,9 @@ namespace RTC

// If not found, ignore.
if (!removedTuple)
{
return;
}

// Notify the listener.
this->listener->OnIceServerTupleRemoved(this, removedTuple);
Expand All @@ -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);
}
Expand Down Expand Up @@ -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");

Expand All @@ -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)
{
Expand All @@ -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);
}
Expand All @@ -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");

Expand All @@ -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)
{
Expand All @@ -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);
}
Expand All @@ -481,7 +507,9 @@ namespace RTC
{
// If a new tuple store it.
if (!HasTuple(tuple))
{
AddTuple(tuple);
}
}
else
{
Expand All @@ -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);
}
Expand All @@ -528,23 +563,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;
}
}
}

Expand All @@ -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);
Expand Down Expand Up @@ -614,22 +658,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;
Expand All @@ -641,7 +684,9 @@ namespace RTC

// If already the selected tuple do nothing.
if (storedTuple == this->selectedTuple)
{
return;
}

this->selectedTuple = storedTuple;

Expand Down
12 changes: 10 additions & 2 deletions worker/src/RTC/WebRtcTransport.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit dad6014

Please sign in to comment.