From eee207d17c210af0efb6f37f0eb6be9784bdb361 Mon Sep 17 00:00:00 2001 From: "guangqing.chen" Date: Sun, 16 May 2021 13:54:33 +0800 Subject: [PATCH 1/9] [core] make sure TTL will not drop packets over last block --- srtcore/buffer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/srtcore/buffer.cpp b/srtcore/buffer.cpp index b29aa9322..bbf02b469 100644 --- a/srtcore/buffer.cpp +++ b/srtcore/buffer.cpp @@ -544,7 +544,7 @@ int CSndBuffer::readData(const int offset, CPacket& w_packet, steady_clock::time w_msglen = 1; p = p->m_pNext; bool move = false; - while (msgno == p->getMsgSeq()) + while (p != m_pLastBlock && msgno == p->getMsgSeq()) { if (p == m_pCurrBlock) move = true; From f25fc0fcbe768d4edde9f7cf93d062d98987dee5 Mon Sep 17 00:00:00 2001 From: "guangqing.chen" Date: Tue, 25 May 2021 18:59:27 +0800 Subject: [PATCH 2/9] address comment --- srtcore/buffer.cpp | 12 ++++++++++++ srtcore/core.cpp | 2 ++ 2 files changed, 14 insertions(+) diff --git a/srtcore/buffer.cpp b/srtcore/buffer.cpp index bbf02b469..24ab7476e 100644 --- a/srtcore/buffer.cpp +++ b/srtcore/buffer.cpp @@ -522,7 +522,19 @@ int CSndBuffer::readData(const int offset, CPacket& w_packet, steady_clock::time // XXX Suboptimal procedure to keep the blocks identifiable // by sequence number. Consider using some circular buffer. for (int i = 0; i < offset; ++i) + { + if (p == NULL || p == m_pLastBlock) + { + LOGC(qslog.Error, log << "CSndBuffer::readData: offset " << offset << " too large!"); + return -2; + } p = p->m_pNext; + } + if (p == NULL || p == m_pLastBlock) + { + LOGC(qslog.Error, log << "CSndBuffer::readData: offset " << offset << " too large!"); + return -2; + } // Check if the block that is the next candidate to send (m_pCurrBlock pointing) is stale. diff --git a/srtcore/core.cpp b/srtcore/core.cpp index a1985379d..13e7982be 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -8886,6 +8886,8 @@ int CUDT::packLostData(CPacket& w_packet, steady_clock::time_point& w_origintime continue; } + else if (payload == -2) + continue; // NOTE: This is just a sanity check. Returning 0 is impossible to happen // in case of retransmission. If the offset was a positive value, then the // block must exist in the old blocks because it wasn't yet cut off by ACK From 7488f5125951c91dcb7cb07a4d1dead61005525e Mon Sep 17 00:00:00 2001 From: "guangqing.chen" Date: Thu, 27 May 2021 21:06:07 +0800 Subject: [PATCH 3/9] no need to check NULL --- srtcore/buffer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/srtcore/buffer.cpp b/srtcore/buffer.cpp index 973fb47b1..0efa5f0f3 100644 --- a/srtcore/buffer.cpp +++ b/srtcore/buffer.cpp @@ -524,14 +524,14 @@ int CSndBuffer::readData(const int offset, srt::CPacket& w_packet, steady_clock: // by sequence number. Consider using some circular buffer. for (int i = 0; i < offset; ++i) { - if (p == NULL || p == m_pLastBlock) + if (p == m_pLastBlock) { LOGC(qslog.Error, log << "CSndBuffer::readData: offset " << offset << " too large!"); return -2; } p = p->m_pNext; } - if (p == NULL || p == m_pLastBlock) + if (p == m_pLastBlock) { LOGC(qslog.Error, log << "CSndBuffer::readData: offset " << offset << " too large!"); return -2; From 506031d7fca1de47cae1f325ad2bf216508e2efe Mon Sep 17 00:00:00 2001 From: "guangqing.chen" Date: Thu, 27 May 2021 21:06:38 +0800 Subject: [PATCH 4/9] add doc for return value --- srtcore/buffer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/srtcore/buffer.h b/srtcore/buffer.h index 9a92e25b0..448c7493d 100644 --- a/srtcore/buffer.h +++ b/srtcore/buffer.h @@ -153,7 +153,7 @@ class CSndBuffer /// @param [out] msgno message number of the packet. /// @param [out] origintime origin time stamp of the message /// @param [out] msglen length of the message - /// @return Actual length of data read. + /// @return Actual length of data read. (-1 if TTL exceeded, -2 if offset too large) int readData(const int offset, srt::CPacket& w_packet, time_point& w_origintime, int& w_msglen); /// Get the time of the last retransmission (if any) of the DATA packet. From 6cbb614c9979bcb058f14f8f8ac383c16c9e336e Mon Sep 17 00:00:00 2001 From: "guangqing.chen" Date: Sat, 29 May 2021 15:21:47 +0800 Subject: [PATCH 5/9] return 0 instead of return -2 --- srtcore/buffer.cpp | 4 ++-- srtcore/buffer.h | 2 +- srtcore/core.cpp | 10 +--------- 3 files changed, 4 insertions(+), 12 deletions(-) diff --git a/srtcore/buffer.cpp b/srtcore/buffer.cpp index 0efa5f0f3..41dd0e584 100644 --- a/srtcore/buffer.cpp +++ b/srtcore/buffer.cpp @@ -527,14 +527,14 @@ int CSndBuffer::readData(const int offset, srt::CPacket& w_packet, steady_clock: if (p == m_pLastBlock) { LOGC(qslog.Error, log << "CSndBuffer::readData: offset " << offset << " too large!"); - return -2; + return 0; } p = p->m_pNext; } if (p == m_pLastBlock) { LOGC(qslog.Error, log << "CSndBuffer::readData: offset " << offset << " too large!"); - return -2; + return 0; } #if ENABLE_HEAVY_LOGGING const int32_t first_seq = p->m_iSeqNo; diff --git a/srtcore/buffer.h b/srtcore/buffer.h index 448c7493d..f69bef1d9 100644 --- a/srtcore/buffer.h +++ b/srtcore/buffer.h @@ -153,7 +153,7 @@ class CSndBuffer /// @param [out] msgno message number of the packet. /// @param [out] origintime origin time stamp of the message /// @param [out] msglen length of the message - /// @return Actual length of data read. (-1 if TTL exceeded, -2 if offset too large) + /// @return Actual length of data read (return 0 if offset too large, -1 if TTL exceeded). int readData(const int offset, srt::CPacket& w_packet, time_point& w_origintime, int& w_msglen); /// Get the time of the last retransmission (if any) of the DATA packet. diff --git a/srtcore/core.cpp b/srtcore/core.cpp index ff879ff96..1b443b3dd 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -8882,7 +8882,6 @@ int srt::CUDT::packLostData(CPacket& w_packet, steady_clock::time_point& w_origi int msglen; const int payload = m_pSndBuffer->readData(offset, (w_packet), (w_origintime), (msglen)); - SRT_ASSERT(payload != 0); if (payload == -1) { int32_t seqpair[2]; @@ -8903,14 +8902,7 @@ int srt::CUDT::packLostData(CPacket& w_packet, steady_clock::time_point& w_origi continue; } - else if (payload == -2) - continue; - // NOTE: This is just a sanity check. Returning 0 is impossible to happen - // in case of retransmission. If the offset was a positive value, then the - // block must exist in the old blocks because it wasn't yet cut off by ACK - // and has been already recorded as sent (otherwise the peer wouldn't send - // back the loss report). May something happen here in case when the send - // loss record has been updated by the FASTREXMIT. + // ignore invalid offset else if (payload == 0) continue; From a25fa942711911c3b5a50c694f36d04b7729dc68 Mon Sep 17 00:00:00 2001 From: "guangqing.chen" Date: Sat, 29 May 2021 15:24:46 +0800 Subject: [PATCH 6/9] add log --- srtcore/core.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/srtcore/core.cpp b/srtcore/core.cpp index 1b443b3dd..05ef13885 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -8902,9 +8902,13 @@ int srt::CUDT::packLostData(CPacket& w_packet, steady_clock::time_point& w_origi continue; } - // ignore invalid offset else if (payload == 0) + { + LOGC(qslog.Error, + log << "IPE/EPE: packLostData: couldn't find a packet with seqno " << w_packet.m_iSeqNo + << " (offset " << offset << ")."); continue; + } // At this point we no longer need the ACK lock, // because we are going to return from the function. From cca7e4b8076b867ba1eee680eafa5c6e54a042aa Mon Sep 17 00:00:00 2001 From: "guangqing.chen" Date: Sat, 29 May 2021 16:36:07 +0800 Subject: [PATCH 7/9] fix typo --- srtcore/core.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/srtcore/core.cpp b/srtcore/core.cpp index 05ef13885..3b99cfc64 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -8904,7 +8904,7 @@ int srt::CUDT::packLostData(CPacket& w_packet, steady_clock::time_point& w_origi } else if (payload == 0) { - LOGC(qslog.Error, + LOGC(qrlog.Error, log << "IPE/EPE: packLostData: couldn't find a packet with seqno " << w_packet.m_iSeqNo << " (offset " << offset << ")."); continue; From c70a1b4c353ac3994310050f38c3881bd3b827b2 Mon Sep 17 00:00:00 2001 From: "guangqing.chen" Date: Sat, 29 May 2021 16:58:09 +0800 Subject: [PATCH 8/9] remove log --- srtcore/core.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/srtcore/core.cpp b/srtcore/core.cpp index 3b99cfc64..826e9c18d 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -8903,12 +8903,7 @@ int srt::CUDT::packLostData(CPacket& w_packet, steady_clock::time_point& w_origi continue; } else if (payload == 0) - { - LOGC(qrlog.Error, - log << "IPE/EPE: packLostData: couldn't find a packet with seqno " << w_packet.m_iSeqNo - << " (offset " << offset << ")."); continue; - } // At this point we no longer need the ACK lock, // because we are going to return from the function. From f8166457df7a9e9642979d2b74234a89b20040e0 Mon Sep 17 00:00:00 2001 From: Maxim Sharabayko Date: Mon, 31 May 2021 11:18:58 +0200 Subject: [PATCH 9/9] For loop with extra check --- srtcore/buffer.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/srtcore/buffer.cpp b/srtcore/buffer.cpp index 41dd0e584..db3102e7a 100644 --- a/srtcore/buffer.cpp +++ b/srtcore/buffer.cpp @@ -522,13 +522,8 @@ int CSndBuffer::readData(const int offset, srt::CPacket& w_packet, steady_clock: // XXX Suboptimal procedure to keep the blocks identifiable // by sequence number. Consider using some circular buffer. - for (int i = 0; i < offset; ++i) + for (int i = 0; i < offset && p != m_pLastBlock; ++i) { - if (p == m_pLastBlock) - { - LOGC(qslog.Error, log << "CSndBuffer::readData: offset " << offset << " too large!"); - return 0; - } p = p->m_pNext; } if (p == m_pLastBlock)