Skip to content

Commit

Permalink
[core] Added proper error handling around sync resources (#1393)
Browse files Browse the repository at this point in the history
  • Loading branch information
ethouris committed Aug 11, 2020
1 parent 8ba040e commit c88ff88
Show file tree
Hide file tree
Showing 6 changed files with 57 additions and 25 deletions.
62 changes: 39 additions & 23 deletions srtcore/api.cpp
Expand Up @@ -199,6 +199,9 @@ m_ClosedSockets()
m_SocketIDGenerator = 1 + int(MAX_SOCKET_VAL * rand1_0);
m_SocketIDGenerator_init = m_SocketIDGenerator;

// XXX An unlikely exception thrown from the below calls
// might destroy the application before `main`. This shouldn't
// be a problem in general.
setupMutex(m_GlobControlLock, "GlobControl");
setupMutex(m_IDLock, "ID");
setupMutex(m_InitLock, "Init");
Expand Down Expand Up @@ -257,8 +260,15 @@ int CUDTUnited::startup()

m_bClosing = false;

setupMutex(m_GCStopLock, "GCStop");
setupCond(m_GCStopCond, "GCStop");
try
{
setupMutex(m_GCStopLock, "GCStop");
setupCond(m_GCStopCond, "GCStop");
}
catch (...)
{
return -1;
}
if (!StartThread(m_GCThread, garbageCollect, this, "SRT:GC"))
return -1;

Expand Down Expand Up @@ -2584,33 +2594,39 @@ void CUDTUnited::updateMux(
// port was specified as 0.
m.m_pChannel->open(addr);
}

sockaddr_any sa;
m.m_pChannel->getSockAddr((sa));
m.m_iPort = sa.hport();
s->m_SelfAddr = sa; // Will be also completed later, but here it's needed for later checks

m.m_pTimer = new CTimer;

m.m_pSndQueue = new CSndQueue;

This comment has been minimized.

Copy link
@elfring

elfring Aug 11, 2020

Can such source code trigger software development concerns according to exception safety?

This comment has been minimized.

Copy link
@maxsharabayko

maxsharabayko Aug 11, 2020

Collaborator

Yes, you are right. m.m_pSndQueue, m.m_pTimer, etc. can be leaking it the exception is thrown.
They have to be deleted inside the catch as well.

This comment has been minimized.

Copy link
@ethouris

ethouris Aug 11, 2020

Author Collaborator

Damn you're right. Of course the memory that was allocated for the object that threw an exception from the constructor will be deallocated automatically - but not the memory for objects that were previously allocated successfully.

This comment has been minimized.

Copy link
@elfring

elfring Aug 11, 2020

Would you like to increase the application of smart pointers?

This comment has been minimized.

Copy link
@maxsharabayko

maxsharabayko Aug 11, 2020

Collaborator

Created issue #1458.

This comment has been minimized.

Copy link
@maxsharabayko

maxsharabayko Aug 11, 2020

Collaborator

Would you like to increase the application of smart pointers?

We have to preserve C++03 compliance of the library. Of course, would make sense to use smart pointers, just C++03 versions will have to be created.
In this case, might be better to create a destructor for CMultiplexer structure. 🤔

m.m_pSndQueue->init(m.m_pChannel, m.m_pTimer);
m.m_pRcvQueue = new CRcvQueue;
m.m_pRcvQueue->init(
32, s->m_pUDT->maxPayloadSize(), m.m_iIPversion, 1024,
m.m_pChannel, m.m_pTimer);

m_mMultiplexer[m.m_iID] = m;

s->m_pUDT->m_pSndQueue = m.m_pSndQueue;
s->m_pUDT->m_pRcvQueue = m.m_pRcvQueue;
s->m_iMuxID = m.m_iID;
}
catch (CUDTException& e)
{
m.m_pChannel->close();
delete m.m_pChannel;
throw;
}

sockaddr_any sa;
m.m_pChannel->getSockAddr((sa));
m.m_iPort = sa.hport();
s->m_SelfAddr = sa; // Will be also completed later, but here it's needed for later checks

m.m_pTimer = new CTimer;

m.m_pSndQueue = new CSndQueue;
m.m_pSndQueue->init(m.m_pChannel, m.m_pTimer);
m.m_pRcvQueue = new CRcvQueue;
m.m_pRcvQueue->init(
32, s->m_pUDT->maxPayloadSize(), m.m_iIPversion, 1024,
m.m_pChannel, m.m_pTimer);

m_mMultiplexer[m.m_iID] = m;

s->m_pUDT->m_pSndQueue = m.m_pSndQueue;
s->m_pUDT->m_pRcvQueue = m.m_pRcvQueue;
s->m_iMuxID = m.m_iID;
catch (...)
{
m.m_pChannel->close();
delete m.m_pChannel;
throw CUDTException(MJ_SYSTEMRES, MN_MEMORY, 0);
}

HLOGF(mglog.Debug,
"creating new multiplexer for port %i\n", m.m_iPort);
Expand Down Expand Up @@ -2827,7 +2843,7 @@ SRTSOCKET CUDT::createGroup(SRT_GROUP_TYPE gt)
{
return APIError(e);
}
catch (const std::bad_alloc& e)
catch (...)
{
return APIError(MJ_SYSTEMRES, MN_MEMORY, 0);
}
Expand Down
1 change: 1 addition & 0 deletions srtcore/cache.h
Expand Up @@ -83,6 +83,7 @@ template<typename T> class CCache
m_iCurrSize(0)
{
m_vHashPtr.resize(m_iHashSize);
// Exception: -> CUDTUnited ctor
srt::sync::setupMutex(m_Lock, "Cache");
}

Expand Down
11 changes: 10 additions & 1 deletion srtcore/core.cpp
Expand Up @@ -3607,7 +3607,16 @@ SRTSOCKET CUDT::makeMePeerOf(SRTSOCKET peergroup, SRT_GROUP_TYPE gtp, uint32_t l
}
else
{
gp = &newGroup(gtp);
try
{
gp = &newGroup(gtp);
}
catch (...)
{
// Expected exceptions are only those referring to system resources
return -1;
}

if (!gp->applyFlags(link_flags, m_SrtHsSide))
{
// Wrong settings. Must reject. Delete group.
Expand Down
1 change: 1 addition & 0 deletions srtcore/epoll.cpp
Expand Up @@ -85,6 +85,7 @@ using namespace srt_logging;
CEPoll::CEPoll():
m_iIDSeed(0)
{
// Exception -> CUDTUnited ctor.
setupMutex(m_EPollLock, "EPoll");
}

Expand Down
6 changes: 5 additions & 1 deletion srtcore/sync.cpp
Expand Up @@ -236,7 +236,11 @@ std::string srt::sync::FormatTimeSys(const steady_clock::time_point& timestamp)
#if !defined(ENABLE_STDCXX_SYNC)
srt::sync::Mutex::Mutex()
{
pthread_mutex_init(&m_mutex, NULL);
const int err = pthread_mutex_init(&m_mutex, 0);
if (err)
{
throw CUDTException(MJ_SYSTEMRES, MN_MEMORY, 0);
}
}

srt::sync::Mutex::~Mutex()
Expand Down
1 change: 1 addition & 0 deletions srtcore/window.h
Expand Up @@ -148,6 +148,7 @@ class CPktTimeWindow: CPktTimeWindowTools
m_tsProbeTime(),
m_Probe1Sequence(SRT_SEQNO_NONE)
{
// Exception: up to CUDT ctor
srt::sync::setupMutex(m_lockPktWindow, "PktWindow");
srt::sync::setupMutex(m_lockProbeWindow, "ProbeWindow");
CPktTimeWindowTools::initializeWindowArrays(m_aPktWindow, m_aProbeWindow, m_aBytesWindow, ASIZE, PSIZE);
Expand Down

0 comments on commit c88ff88

Please sign in to comment.