Skip to content

Commit

Permalink
[core] Fixed bug: finding listener's muxer by port is wrong (#1500)
Browse files Browse the repository at this point in the history 

Co-authored-by: rexvl <anton.korovin@gmail.com>
  • Loading branch information
@ethouris @rexvl
ethouris and rexvl authored Aug 31, 2020
1 parent a755d8f commit 333d34f
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 59 deletions.
137 changes: 79 additions & 58 deletions srtcore/api.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2645,71 +2645,92 @@ void CUDTUnited::updateMux(
"creating new multiplexer for port %i\n", m.m_iPort);
}

// XXX This functionality needs strong refactoring.
//
// This function is going to find a multiplexer for the port contained
// in the 'ls' listening socket, by searching through the multiplexer
// container.
//
// Somehow, however, it's not even predicted a situation that the multiplexer
// for that port doesn't exist - that is, this function WILL find the
// multiplexer. How can it be so certain? It's because the listener has
// already created the multiplexer during the call to bind(), so if it
// didn't, this function wouldn't even have a chance to be called.
//
// Why can't then the multiplexer be recorded in the 'ls' listening socket data
// to be accessed immediately, especially when one listener can't bind to more
// than one multiplexer at a time (well, even if it could, there's still no
// reason why this should be extracted by "querying")?
//
// Maybe because the multiplexer container is a map, not a list.
// Why is this then a map? Because it's addressed by MuxID. Why do we need
// mux id? Because we don't have a list... ?
//
// But what's the multiplexer ID? It's a socket ID for which it was originally
// created.
//
// Is this then shared? Yes, only between the listener socket and the accepted
// sockets, or in case of "bound" connecting sockets (by binding you can
// enforce the port number, which can be the same for multiple SRT sockets).
// Not shared in case of unbound connecting socket or rendezvous socket.
//
// Ok, in which situation do we need dispatching by mux id? Only when the
// socket is being deleted. How does the deleting procedure know the muxer id?
// Because it is recorded here at the time when it's found, as... the socket ID
// of the actual listener socket being actually the first socket to create the
// multiplexer, so the multiplexer gets its id.
//
// Still, no reasons found why the socket can't contain a list iterator to a
// multiplexer INSTEAD of m_iMuxID. There's no danger in this solution because
// the multiplexer is never deleted until there's at least one socket using it.
//
// The multiplexer may even physically be contained in the CUDTUnited object,
// just track the multiple users of it (the listener and the accepted sockets).
// When deleting, you simply "unsubscribe" yourself from the multiplexer, which
// will unref it and remove the list element by the iterator kept by the
// socket.
void CUDTUnited::updateListenerMux(CUDTSocket* s, const CUDTSocket* ls)
// in the 'ls' listening socket. The multiplexer must exist when the listener
// exists, otherwise the dispatching procedure wouldn't even call this
// function. By historical reasons there's also a fallback for a case when the
// multiplexer wasn't found by id, the search by port number continues.
bool CUDTUnited::updateListenerMux(CUDTSocket* s, const CUDTSocket* ls)
{
ScopedLock cg(m_GlobControlLock);
const int port = ls->m_SelfAddr.hport();

// find the listener's address
for (map<int, CMultiplexer>::iterator i = m_mMultiplexer.begin();
i != m_mMultiplexer.end(); ++ i)
HLOGC(smlog.Debug, log << "updateListenerMux: finding muxer of listener socket @"
<< ls->m_SocketID << " muxid=" << ls->m_iMuxID
<< " bound=" << ls->m_SelfAddr.str()
<< " FOR @" << s->m_SocketID << " addr="
<< s->m_SelfAddr.str() << "_->_" << s->m_PeerAddr.str());

// First thing that should be certain here is that there should exist
// a muxer with the ID written in the listener socket's mux ID.

CMultiplexer* mux = map_getp(m_mMultiplexer, ls->m_iMuxID);

// NOTE:
// THIS BELOW CODE is only for a highly unlikely, and probably buggy,
// situation when the Multiplexer wasn't found by ID recorded in the listener.
CMultiplexer* fallback = NULL;
if (!mux)
{
if (i->second.m_iPort == port)
{
HLOGF(smlog.Debug,
"updateMux: reusing multiplexer for port %i\n", port);
// reuse the existing multiplexer
++ i->second.m_iRefCount;
s->m_pUDT->m_pSndQueue = i->second.m_pSndQueue;
s->m_pUDT->m_pRcvQueue = i->second.m_pRcvQueue;
s->m_iMuxID = i->second.m_iID;
return;
}
LOGC(smlog.Error, log << "updateListenerMux: IPE? listener muxer not found by ID, trying by port");

// To be used as first found with different IP version

// find the listener's address
for (map<int, CMultiplexer>::iterator i = m_mMultiplexer.begin();
i != m_mMultiplexer.end(); ++ i)
{
CMultiplexer& m = i->second;

#if ENABLE_HEAVY_LOGGING
ostringstream that_muxer;
that_muxer << "id=" << m.m_iID
<< " port=" << m.m_iPort
<< " ip=" << (m.m_iIPversion == AF_INET ? "v4" : "v6");
#endif

if (m.m_iPort == port)
{
HLOGC(smlog.Debug, log << "updateListenerMux: reusing muxer: " << that_muxer.str());
if (m.m_iIPversion == s->m_PeerAddr.family())
{
mux = &m; // best match
break;
}
else
{
fallback = &m;
}
}
else
{
HLOGC(smlog.Debug, log << "updateListenerMux: SKIPPING muxer: " << that_muxer.str());
}
}

if (!mux && fallback)
{
// It is allowed to reuse this multiplexer, but the socket must allow both IPv4 and IPv6
if (fallback->m_iIpV6Only == 0)
{
HLOGC(smlog.Warn, log << "updateListenerMux: reusing multiplexer from different family");
mux = fallback;
}
}
}

// Checking again because the above procedure could have set it
if (mux)
{
// reuse the existing multiplexer
++ mux->m_iRefCount;
s->m_pUDT->m_pSndQueue = mux->m_pSndQueue;
s->m_pUDT->m_pRcvQueue = mux->m_pRcvQueue;
s->m_iMuxID = mux->m_iID;
return true;
}

return false;
}

void* CUDTUnited::garbageCollect(void* p)
Expand Down
13 changes: 12 additions & 1 deletion srtcore/api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,17 @@ class CUDTSocket

unsigned int m_uiBackLog; //< maximum number of connections in queue

// XXX A refactoring might be needed here.

// There are no reasons found why the socket can't contain a list iterator to a
// multiplexer INSTEAD of m_iMuxID. There's no danger in this solution because
// the multiplexer is never deleted until there's at least one socket using it.
//
// The multiplexer may even physically be contained in the CUDTUnited object,
// just track the multiple users of it (the listener and the accepted sockets).
// When deleting, you simply "unsubscribe" yourself from the multiplexer, which
// will unref it and remove the list element by the iterator kept by the
// socket.
int m_iMuxID; //< multiplexer ID

srt::sync::Mutex m_ControlLock; //< lock this socket exclusively for control APIs: bind/listen/connect
Expand Down Expand Up @@ -346,7 +357,7 @@ friend class CRendezvousQueue;
CUDTSocket* locatePeer(const sockaddr_any& peer, const SRTSOCKET id, int32_t isn);
CUDTGroup* locateGroup(SRTSOCKET u, ErrorHandling erh = ERH_RETURN);
void updateMux(CUDTSocket* s, const sockaddr_any& addr, const UDPSOCKET* = NULL);
void updateListenerMux(CUDTSocket* s, const CUDTSocket* ls);
bool updateListenerMux(CUDTSocket* s, const CUDTSocket* ls);

private:
std::map<int, CMultiplexer> m_mMultiplexer; // UDP multiplexer
Expand Down

0 comments on commit 333d34f

Please sign in to comment.