Skip to content

Commit

Permalink
[core] FIX: Added check for NULL unit when passing to updateConnStatus (
Browse files Browse the repository at this point in the history 
#2028)
  • Loading branch information
@ethouris
ethouris authored May 31, 2021
1 parent 17fee15 commit 1751bab
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 30 deletions.
48 changes: 30 additions & 18 deletions srtcore/core.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3600,7 +3600,7 @@ void srt::CUDT::startConnect(const sockaddr_any& serv_addr, int32_t forced_isn)
// it means that it has done all that was required, however none of the below
// things has to be done (this function will do it by itself if needed).
// Otherwise the handshake rolling can be interrupted and considered complete.
cst = processRendezvous(response, serv_addr, RST_OK, (reqpkt));
cst = processRendezvous(&response, serv_addr, RST_OK, (reqpkt));
if (cst == CONN_CONTINUE)
continue;
break;
Expand Down Expand Up @@ -3744,7 +3744,7 @@ EConnectStatus srt::CUDT::processAsyncConnectResponse(const CPacket &pkt) ATR_NO

bool srt::CUDT::processAsyncConnectRequest(EReadStatus rst,
EConnectStatus cst,
const CPacket& response,
const CPacket* pResponse /*[[nullable]]*/,
const sockaddr_any& serv_addr)
{
// IMPORTANT!
Expand Down Expand Up @@ -3773,7 +3773,7 @@ bool srt::CUDT::processAsyncConnectRequest(EReadStatus rst,
if (cst == CONN_RENDEZVOUS)
{
HLOGC(cnlog.Debug, log << "processAsyncConnectRequest: passing to processRendezvous");
cst = processRendezvous(response, serv_addr, rst, (request));
cst = processRendezvous(pResponse, serv_addr, rst, (request));
if (cst == CONN_ACCEPT)
{
HLOGC(cnlog.Debug,
Expand Down Expand Up @@ -3976,7 +3976,7 @@ EConnectStatus srt::CUDT::craftKmResponse(uint32_t* aw_kmdata, size_t& w_kmdatas
}

EConnectStatus srt::CUDT::processRendezvous(
const CPacket& response, const sockaddr_any& serv_addr,
const CPacket* pResponse /*[[nullable]]*/, const sockaddr_any& serv_addr,
EReadStatus rst, CPacket& w_reqpkt)
{
if (m_RdvState == CHandShake::RDV_CONNECTED)
Expand Down Expand Up @@ -4053,15 +4053,15 @@ EConnectStatus srt::CUDT::processRendezvous(
// We have JUST RECEIVED packet in this session (not that this is called as periodic update).
// Sanity check
m_tsLastReqTime = steady_clock::time_point();
if (response.getLength() == size_t(-1))
if (!pResponse || pResponse->getLength() == size_t(-1))
{
m_RejectReason = SRT_REJ_IPE;
LOGC(cnlog.Fatal,
log << "IPE: rst=RST_OK, but the packet has set -1 length - REJECTING (REQ-TIME: LOW)");
return CONN_REJECT;
}

if (!interpretSrtHandshake(m_ConnRes, response, kmdata, &kmdatasize))
if (!interpretSrtHandshake(m_ConnRes, *pResponse, kmdata, &kmdatasize))
{
HLOGC(cnlog.Debug,
log << "processRendezvous: rejecting due to problems in interpretSrtHandshake REQ-TIME: LOW.");
Expand Down Expand Up @@ -4114,7 +4114,7 @@ EConnectStatus srt::CUDT::processRendezvous(
// not be done in case of rendezvous. The section in postConnect() is
// predicted to run only in regular CALLER handling.

if (rst != RST_OK || response.getLength() == size_t(-1))
if (rst != RST_OK || !pResponse || pResponse->getLength() == size_t(-1))
{
// Actually the -1 length would be an IPE, but it's likely that this was reported already.
HLOGC(
Expand All @@ -4125,7 +4125,7 @@ EConnectStatus srt::CUDT::processRendezvous(
{
HLOGC(cnlog.Debug,
log << "processRendezvous: INITIATOR, will send AGREEMENT - interpreting HSRSP extension");
if (!interpretSrtHandshake(m_ConnRes, response, 0, 0))
if (!interpretSrtHandshake(m_ConnRes, *pResponse, 0, 0))
{
// m_RejectReason is already set, so set the reqtype accordingly
m_ConnReq.m_iReqType = URQFailure(m_RejectReason);
Expand Down Expand Up @@ -4165,10 +4165,7 @@ EConnectStatus srt::CUDT::processRendezvous(
w_reqpkt.setLength(m_iMaxSRTPayloadSize);
if (m_RdvState == CHandShake::RDV_CONNECTED)
{
// When synchro=false, don't lock a mutex for rendezvous queue.
// This is required when this function is called in the
// receive queue worker thread - it would lock itself.
int cst = postConnect(response, true, 0);
int cst = postConnect(pResponse, true, 0);
if (cst == CONN_REJECT)
{
// m_RejectReason already set
Expand Down Expand Up @@ -4304,7 +4301,7 @@ EConnectStatus srt::CUDT::processConnectResponse(const CPacket& response, CUDTEx
m_RdvState = CHandShake::RDV_CONNECTED;
}

return postConnect(response, hsv5, eout);
return postConnect(&response, hsv5, eout);
}

if (!response.isControl(UMSG_HANDSHAKE))
Expand Down Expand Up @@ -4474,7 +4471,7 @@ EConnectStatus srt::CUDT::processConnectResponse(const CPacket& response, CUDTEx
}
}

return postConnect(response, false, eout);
return postConnect(&response, false, eout);
}

bool srt::CUDT::applyResponseSettings() ATR_NOEXCEPT
Expand Down Expand Up @@ -4507,7 +4504,7 @@ bool srt::CUDT::applyResponseSettings() ATR_NOEXCEPT
return true;
}

EConnectStatus srt::CUDT::postConnect(const CPacket &response, bool rendezvous, CUDTException *eout) ATR_NOEXCEPT
EConnectStatus srt::CUDT::postConnect(const CPacket* pResponse, bool rendezvous, CUDTException *eout) ATR_NOEXCEPT
{
if (m_ConnRes.m_iVersion < HS_VERSION_SRT1)
m_tsRcvPeerStartTime = steady_clock::time_point(); // will be set correctly in SRT HS.
Expand All @@ -4516,6 +4513,21 @@ EConnectStatus srt::CUDT::postConnect(const CPacket &response, bool rendezvous,
// in rendezvous it's completed before calling this function.
if (!rendezvous)
{
// The "local storage depleted" case shouldn't happen here, but
// this is a theoretical path that needs prevention.
bool ok = pResponse;
if (!ok)
{
m_RejectReason = SRT_REJ_IPE;
if (eout)
{
*eout = CUDTException(MJ_SETUP, MN_REJECTED, 0);
}
return CONN_REJECT;
}

// [[assert (pResponse != NULL)]];

// NOTE: THIS function must be called before calling prepareConnectionObjects.
// The reason why it's not part of prepareConnectionObjects is that the activities
// done there are done SIMILAR way in acceptAndRespond, which also calls this
Expand All @@ -4527,7 +4539,7 @@ EConnectStatus srt::CUDT::postConnect(const CPacket &response, bool rendezvous,
//
// Currently just this function must be called always BEFORE prepareConnectionObjects
// everywhere except acceptAndRespond().
bool ok = applyResponseSettings();
ok = applyResponseSettings();

// This will actually be done also in rendezvous HSv4,
// however in this case the HSREQ extension will not be attached,
Expand All @@ -4537,8 +4549,8 @@ EConnectStatus srt::CUDT::postConnect(const CPacket &response, bool rendezvous,

// May happen that 'response' contains a data packet that was sent in rendezvous mode.
// In this situation the interpretation of handshake was already done earlier.
ok = ok && response.isControl();
ok = ok && interpretSrtHandshake(m_ConnRes, response, 0, 0);
ok = ok && pResponse->isControl();
ok = ok && interpretSrtHandshake(m_ConnRes, *pResponse, 0, 0);

if (!ok)
{
Expand Down
6 changes: 3 additions & 3 deletions srtcore/core.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -475,12 +475,12 @@ class CUDT
/// @param response incoming handshake response packet to be interpreted
/// @param serv_addr incoming packet's address
/// @param rst Current read status to know if the HS packet was freshly received from the peer, or this is only a periodic update (RST_AGAIN)
SRT_ATR_NODISCARD EConnectStatus processRendezvous(const CPacket &response, const sockaddr_any& serv_addr, EReadStatus, CPacket& reqpkt);
SRT_ATR_NODISCARD EConnectStatus processRendezvous(const CPacket* response, const sockaddr_any& serv_addr, EReadStatus, CPacket& reqpkt);
SRT_ATR_NODISCARD bool prepareConnectionObjects(const CHandShake &hs, HandshakeSide hsd, CUDTException *eout);
SRT_ATR_NODISCARD EConnectStatus postConnect(const CPacket& response, bool rendezvous, CUDTException* eout) ATR_NOEXCEPT;
SRT_ATR_NODISCARD EConnectStatus postConnect(const CPacket* response, bool rendezvous, CUDTException* eout) ATR_NOEXCEPT;
SRT_ATR_NODISCARD bool applyResponseSettings() ATR_NOEXCEPT;
SRT_ATR_NODISCARD EConnectStatus processAsyncConnectResponse(const CPacket& pkt) ATR_NOEXCEPT;
SRT_ATR_NODISCARD bool processAsyncConnectRequest(EReadStatus rst, EConnectStatus cst, const CPacket& response, const sockaddr_any& serv_addr);
SRT_ATR_NODISCARD bool processAsyncConnectRequest(EReadStatus rst, EConnectStatus cst, const CPacket* response, const sockaddr_any& serv_addr);
SRT_ATR_NODISCARD EConnectStatus craftKmResponse(uint32_t* aw_kmdata, size_t& w_kmdatasize);

void checkUpdateCryptoKeyLen(const char* loghdr, int32_t typefield);
Expand Down
20 changes: 12 additions & 8 deletions srtcore/queue.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -903,17 +903,21 @@ srt::CUDT* srt::CRendezvousQueue::retrieve(const sockaddr_any& addr, SRTSOCKET&
return NULL;
}

void srt::CRendezvousQueue::updateConnStatus(EReadStatus rst, EConnectStatus cst, const CPacket& pktIn)
void srt::CRendezvousQueue::updateConnStatus(EReadStatus rst, EConnectStatus cst, CUnit* unit)
{
vector<LinkStatusInfo> toRemove, toProcess;

const CPacket* pkt = unit ? &unit->m_Packet : NULL;

// Need a stub value for a case when there's no unit provided ("storage depleted" case).
// It should be normally NOT IN USE because in case of "storage depleted", rst != RST_OK.
const SRTSOCKET dest_id = pkt ? pkt->m_iID : 0;

// If no socket were qualified for further handling, finish here.
// Otherwise toRemove and toProcess contain items to handle.
if (!qualifyToHandle(rst, cst, pktIn.m_iID, toRemove, toProcess))
if (!qualifyToHandle(rst, cst, dest_id, (toRemove), (toProcess)))
return;

// [[using locked()]];

HLOGC(cnlog.Debug,
log << "updateConnStatus: collected " << toProcess.size() << " for processing, " << toRemove.size()
<< " to close");
Expand All @@ -938,7 +942,7 @@ void srt::CRendezvousQueue::updateConnStatus(EReadStatus rst, EConnectStatus cst
EReadStatus read_st = rst;
EConnectStatus conn_st = cst;

if (i->id != pktIn.m_iID)
if (i->id != dest_id)
{
read_st = RST_AGAIN;
conn_st = CONN_AGAIN;
Expand All @@ -947,7 +951,7 @@ void srt::CRendezvousQueue::updateConnStatus(EReadStatus rst, EConnectStatus cst
HLOGC(cnlog.Debug,
log << "updateConnStatus: processing async conn for @" << i->id << " FROM " << i->peeraddr.str());

if (!i->u->processAsyncConnectRequest(read_st, conn_st, pktIn, i->peeraddr))
if (!i->u->processAsyncConnectRequest(read_st, conn_st, pkt, i->peeraddr))
{
// cst == CONN_REJECT can only be result of worker_ProcessAddressedPacket and
// its already set in this case.
Expand Down Expand Up @@ -1310,7 +1314,7 @@ void* srt::CRcvQueue::worker(void* param)
// worker_TryAsyncRend_OrStore --->
// CUDT::processAsyncConnectResponse --->
// CUDT::processConnectResponse
self->m_pRendezvousQueue->updateConnStatus(rst, cst, unit->m_Packet);
self->m_pRendezvousQueue->updateConnStatus(rst, cst, unit);

// XXX updateConnStatus may have removed the connector from the list,
// however there's still m_mBuffer in CRcvQueue for that socket to care about.
Expand Down Expand Up @@ -1526,7 +1530,7 @@ EConnectStatus srt::CRcvQueue::worker_TryAsyncRend_OrStore(int32_t id, CUnit* un
{
LOGC(cnlog.Warn, log << "AsyncOrRND: PACKET NOT HANDSHAKE - re-requesting handshake from peer");
storePkt(id, unit->m_Packet.clone());
if (!u->processAsyncConnectRequest(RST_AGAIN, CONN_CONTINUE, unit->m_Packet, u->m_PeerAddr))
if (!u->processAsyncConnectRequest(RST_AGAIN, CONN_CONTINUE, &unit->m_Packet, u->m_PeerAddr))
{
// Reuse previous behavior to reject a packet
cst = CONN_REJECT;
Expand Down
2 changes: 1 addition & 1 deletion srtcore/queue.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ class CRendezvousQueue
/// @param rst result of reading from a UDP socket: received packet / nothin read / read error.
/// @param cst target status for pending connection: reject or proceed.
/// @param pktIn packet received from the UDP socket.
void updateConnStatus(EReadStatus rst, EConnectStatus cst, const CPacket& pktIn);
void updateConnStatus(EReadStatus rst, EConnectStatus cst, CUnit* unit);

private:
struct LinkStatusInfo
Expand Down

0 comments on commit 1751bab

Please sign in to comment.