Skip to content

Commit

Permalink
-fix bug with SipStack::removeTransport method - transport selector m…
Browse files Browse the repository at this point in the history
…aps could get out of sync and cause existing transports

 to not get used correctly after a runtime removal
  • Loading branch information
Scott Godin committed Oct 25, 2016
1 parent 4252282 commit b768c4c
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 7 deletions.
8 changes: 5 additions & 3 deletions resip/stack/SipStack.cxx
Expand Up @@ -1247,9 +1247,11 @@ SipStack::dump(EncodeStream& strm) const
}
strm << " ServerTransactionMap size=" << this->mTransactionController->mServerTransactionMap.size() << std::endl
<< " ClientTransactionMap size=" << this->mTransactionController->mClientTransactionMap.size() << std::endl
// !slg! TODO - There is technically a threading concern with the following three lines and the runtime addTransport call
<< " Exact Transports=" << Inserter(this->mTransactionController->mTransportSelector.mExactTransports) << std::endl
<< " Any Transports=" << Inserter(this->mTransactionController->mTransportSelector.mAnyInterfaceTransports) << std::endl
// !slg! TODO - There is technically a threading concern with the following three lines and the runtime addTransport or removeTransport call
<< " Exact interface / Specific port=" << Inserter(this->mTransactionController->mTransportSelector.mExactTransports) << std::endl
<< " Any interface / Specific port=" << Inserter(this->mTransactionController->mTransportSelector.mAnyInterfaceTransports) << std::endl
<< " Exact interface / Any port =" << Inserter(this->mTransactionController->mTransportSelector.mAnyPortTransports) << std::endl
<< " Any interface / Any port=" << Inserter(this->mTransactionController->mTransportSelector.mAnyPortAnyInterfaceTransports) << std::endl
<< " TLS Transports=" << Inserter(this->mTransactionController->mTransportSelector.mTlsTransports) << std::endl;
return strm;
}
Expand Down
55 changes: 51 additions & 4 deletions resip/stack/TransportSelector.cxx
Expand Up @@ -299,6 +299,8 @@ TransportSelector::removeTransport(unsigned int transportKey)
Transport* transportToRemove = 0;

// Find transport in global map and remove it
// Note: it is important that this is map is removed from before rebuildAnyPortTransportMaps is called,
// since it uses this map.
TransportKeyMap::iterator it = mTransports.find(transportKey);
if(it != mTransports.end())
{
Expand All @@ -315,10 +317,12 @@ TransportSelector::removeTransport(unsigned int transportKey)
if(!isSecure(transportToRemove->transport()))
{
// Ensure transport is removed from all containers
mAnyInterfaceTransports.erase(transportToRemove->getTuple());
mAnyPortAnyInterfaceTransports.erase(transportToRemove->getTuple());
mExactTransports.erase(transportToRemove->getTuple());
mAnyPortTransports.erase(transportToRemove->getTuple());
mAnyInterfaceTransports.erase(transportToRemove->getTuple());

// In the AnyPort maps 2 transports can end up overwriting each other in these maps - then when we remove one, there may be none left - even though we should have an
// entry. The rebuilt method will dig through all transports again and rebuild these maps.
rebuildAnyPortTransportMaps();
}
else
{
Expand All @@ -328,9 +332,19 @@ TransportSelector::removeTransport(unsigned int transportKey)
mTlsTransports.erase(tlsKey);
}

mTypeToTransportMap.erase(transportToRemove->getTuple());
// mTypeToTransportMap is a multimap - make sure to delete only this instance by looking up transportKey, instead of using
// mTypeToTransportMap.erase(transportToRemove->getTuple()); which might end up deleting more than 1 transport
for (TypeToTransportMap::iterator itTypeToTransport = mTypeToTransportMap.begin(); itTypeToTransport != mTypeToTransportMap.end(); itTypeToTransport++)
{
if (itTypeToTransport->second->getKey() == transportKey)
{
mTypeToTransportMap.erase(itTypeToTransport);
break;
}
}

// Remove transport types from Dns list of supported protocols
// Note: DNS tracks use counts so that we will only remove this transport type if this is the last of the type to be removed
mDns.removeTransportType(transportToRemove->transport(), transportToRemove->ipVersion());

if (transportToRemove->shareStackProcessAndSelect())
Expand All @@ -357,6 +371,39 @@ TransportSelector::removeTransport(unsigned int transportKey)
}
}

void
TransportSelector::rebuildAnyPortTransportMaps()
{
// These maps may contain less transports than what exist in the mTransports map, due to the fact that multiple transports can
// match their index. In these cases the last transport added that matchs the map compare function is the only one that ends
// up in these maps. Therefor we cannot just simply remove items and expect transprot selection to work as expects.
// We will clear these maps here. Iterate through the master transport list and rebuilt them back up. This isn't very efficient,
// but it only occurs when a transport is removed.

mAnyPortTransports.clear();
mAnyPortAnyInterfaceTransports.clear();

for (TransportKeyMap::iterator it = mTransports.begin(); it != mTransports.end(); it++)
{
if (!isSecure(it->second->transport()))
{
// Store the transport in the ANY interface maps if the tuple specifies ANY
// interface. Store the transport in the specific interface maps if the tuple
// specifies an interface. See TransportSelector::findTransport.
if (it->second->interfaceName().empty() ||
it->second->getTuple().isAnyInterface() ||
it->second->hasSpecificContact())
{
mAnyPortAnyInterfaceTransports[it->second->getTuple()] = it->second;
}
else
{
mAnyPortTransports[it->second->getTuple()] = it->second;
}
}
}
}

void
TransportSelector::setPollGrp(FdPollGrp *grp)
{
Expand Down
1 change: 1 addition & 0 deletions resip/stack/TransportSelector.hxx
Expand Up @@ -202,6 +202,7 @@ class TransportSelector
Transport* findTransportByVia(SipMessage* msg, const Tuple& dest, Tuple& src) const;
Transport* findTlsTransport(const Data& domain,TransportType type,IpVersion ipv) const;
Tuple determineSourceInterface(SipMessage* msg, const Tuple& dest) const;
void rebuildAnyPortTransportMaps(void);

DnsInterface mDns;
Fifo<TransactionMessage>& mStateMacFifo;
Expand Down

0 comments on commit b768c4c

Please sign in to comment.