From fc943ac47e341216cc47695318fecb6f35adcad1 Mon Sep 17 00:00:00 2001 From: Maxim Sharabayko Date: Mon, 6 Feb 2023 17:25:24 +0100 Subject: [PATCH 1/2] [core] RCV erase undecrypted packet instead of dropping. On a decryption failure the packet is dropped from the receiver buffer, which marks the buffer slot as dropped. That prevents a valid packet with the same sequence number, should it arrive later, from being stored. Erase it instead: the slot is left empty and therefore fillable. TODO: Erasing the packet means it has to be added back to the loss list. Originally submitted as #2649; rebased here onto master and adapted to the current CRcvBuffer API (dropUpTo signature, EntryState entries) and to the statistics/frequent-log block introduced by #2654. --- srtcore/buffer_rcv.cpp | 42 ++++++++++++++++++++++++++++++++++++++++++ srtcore/buffer_rcv.h | 6 ++++++ srtcore/core.cpp | 17 ++++++++++------- 3 files changed, 58 insertions(+), 7 deletions(-) diff --git a/srtcore/buffer_rcv.cpp b/srtcore/buffer_rcv.cpp index 88f94103b..dfab3e7ca 100644 --- a/srtcore/buffer_rcv.cpp +++ b/srtcore/buffer_rcv.cpp @@ -206,6 +206,48 @@ int CRcvBuffer::insert(CUnit* unit) return 0; } +int CRcvBuffer::erase(int32_t seqno) +{ + const int offset = CSeqNo::seqoff(m_iStartSeqNo, seqno); + if (offset < 0) + { + LOGC(rbuflog.Debug, log << "CRcvBuffer.erase(): nothing to erase. Requested @" << seqno + << ". Buffer start " << m_iStartSeqNo << "."); + return 0; + } + + const int pos = incPos(m_iStartPos, offset); + if (!m_entries[pos].pUnit) + return 0; + + const bool bMsgOrderFlag = packetAt(pos).getMsgOrderFlag(); + // Leaves the entry EntryState_Empty, so that a retransmission of the same + // sequence number can still be inserted into this slot. + releaseUnitInPos(pos); + + if (m_bMessageAPI && !bMsgOrderFlag && !m_tsbpd.isEnabled()) + { + --m_numOutOfOrderPackets; + if (pos == m_iFirstReadableOutOfOrder) + { + m_iFirstReadableOutOfOrder = -1; + updateFirstReadableOutOfOrder(); + } + } + + HLOGC(rbuflog.Debug, log << "CRcvBuffer.erase(): @" << seqno << "."); + + // Check if a unit before m_iFirstNonreadPos was erased. + const bool needUpdateNonreadPos = offset <= getRcvDataSize(); + if (needUpdateNonreadPos) + { + m_iFirstNonreadPos = m_iStartPos; + updateNonreadPos(); + } + + return 1; +} + std::pair CRcvBuffer::dropUpTo(int32_t seqno) { IF_RCVBUF_DEBUG(ScopedLog scoped_log); diff --git a/srtcore/buffer_rcv.h b/srtcore/buffer_rcv.h index eac1c7c47..9fcbe92f7 100644 --- a/srtcore/buffer_rcv.h +++ b/srtcore/buffer_rcv.h @@ -64,6 +64,12 @@ class CRcvBuffer // TODO: Previously '-2' also meant 'already acknowledged'. Check usage of this value. int insert(CUnit* unit); + /// Erase a packet from the buffer based on the packet sequence number. + /// The entry is marked EntryState_Empty. The CUnit is marked free. + /// @param seqno packet sequence number. + /// @return the number of packets erased. + int erase(int32_t seqno); + /// Drop packets in the receiver buffer from the current position up to the seqno (excluding seqno). /// @param [in] seqno drop units up to this sequence number /// @return number of dropped (missing) and discarded (available) packets as a pair(dropped, discarded). diff --git a/srtcore/core.cpp b/srtcore/core.cpp index ece511b3e..2d2521988 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -10750,20 +10750,23 @@ int srt::CUDT::handleSocketPacketReception(const vector& incoming, bool& // See issue ##2626. SRT_ASSERT(m_bTsbPd); - // Drop the packet from the receiver buffer. - // The packet was added to the buffer based on the sequence number, therefore sequence number should be used to drop it from the buffer. - // A drawback is that it would prevent a valid packet with the same sequence number, if it happens to arrive later, to end up in the buffer. - const int iDropCnt = m_pRcvBuffer->dropMessage(u->m_Packet.getSeqNo(), u->m_Packet.getSeqNo(), SRT_MSGNO_NONE, CRcvBuffer::DROP_EXISTING); + // Erase the packet from the receiver buffer, rather than dropping it. + // Dropping marks the buffer slot as dropped, which would prevent a valid packet + // with the same sequence number, if it happens to arrive later, from ending up + // in the buffer. Erasing leaves the slot fillable. + // The packet was added to the buffer based on the sequence number, therefore the + // sequence number is used to erase it from the buffer. + // TODO: Erasing the packet means it has to be added back to the loss list. + const int iEraseCnt = m_pRcvBuffer->erase(u->m_Packet.getSeqNo()); const steady_clock::time_point tnow = steady_clock::now(); ScopedLock lg(m_StatsLock); - m_stats.rcvr.dropped.count(stats::BytesPackets(iDropCnt * rpkt.getLength(), iDropCnt)); m_stats.rcvr.undecrypted.count(stats::BytesPackets(rpkt.getLength(), 1)); string why; if (frequentLogAllowed(FREQLOGFA_ENCRYPTION_FAILURE, tnow, (why))) { - LOGC(qrlog.Warn, log << CONID() << "Decryption failed (seqno %" << u->m_Packet.getSeqNo() << "), dropped " - << iDropCnt << ". pktRcvUndecryptTotal=" << m_stats.rcvr.undecrypted.total.count() << "." << why); + LOGC(qrlog.Warn, log << CONID() << "Decryption failed (seqno %" << u->m_Packet.getSeqNo() << "), erased " + << iEraseCnt << ". pktRcvUndecryptTotal=" << m_stats.rcvr.undecrypted.total.count() << "." << why); } #if SRT_ENABLE_FREQUENT_LOG_TRACE else From faf2f62d6df28aaa1e0d4ca4d1b3448e8423a33c Mon Sep 17 00:00:00 2001 From: danryu Date: Sun, 26 Jul 2026 13:40:45 +0200 Subject: [PATCH 2/2] [core] RCV re-request undecrypted (AEAD) packets instead of losing them Completes the TODO left by the previous commit. With AEAD (AES-GCM) the receiver can detect a payload corrupted in transit - the auth tag fails - but the packet was then only erased, never re-requested, so it stayed a permanent gap in the delivered stream. For applications that need bit-exact delivery over paths that produce corruption surviving the UDP checksum, AEAD is currently detect-but-not-recover: a silent corruption becomes a dropout. Treat an undecryptable packet exactly like a lost one. The payload is erased (slot kept fillable), the sequence stays in or goes back into the receiver loss list, a NAK is sent, and the existing ARQ machinery retransmits the good copy into the held-open slot. No sender-side change and no new machinery. Four receiver-side parts, three of which were only found under adversarial testing and are the reason completing the TODO literally is not enough: - Undo insert()'s countBytes() accounting in CRcvBuffer::erase(). Without this the retransmission landing in the freed slot double-counts buffer bytes and packets. - Push the erased sequence into the loss output, AFTER the loss-detection block. CRcvLossList::insert() rejects entries below its largest-ever sequence, so a preceding sequence-jump range must be recorded first. - Do not unlose() an undecrypted packet. The arrival of a corrupt retransmission removed the sequence from the loss list and the re-add was then rejected by the monotonic guard, leaving it in no loss list at all. Since the ACK position is derived from that list, the next full ACK advanced past the erased hole and every later copy - corrupt or clean - was discarded as belated, making the sequence unrecoverable. Measured before the fix: ~5-6% of corrupt-retransmit rounds died this way. - Run sequence-jump loss detection for undecrypted packets too. The check was gated on adding_successful (false on decrypt failure) while m_iRcvCurrSeqNo advanced regardless, so packets genuinely lost immediately before a corrupt arrival were never recorded or NAKed and became permanent gaps. This defect is reachable in the existing GCM path on its own, independently of recovery. Every new branch is conditional on an AEAD decryption failure; on clean and on unencrypted streams the code is inert. --- srtcore/buffer_rcv.cpp | 4 ++++ srtcore/core.cpp | 30 ++++++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/srtcore/buffer_rcv.cpp b/srtcore/buffer_rcv.cpp index dfab3e7ca..45f9ecff6 100644 --- a/srtcore/buffer_rcv.cpp +++ b/srtcore/buffer_rcv.cpp @@ -221,6 +221,10 @@ int CRcvBuffer::erase(int32_t seqno) return 0; const bool bMsgOrderFlag = packetAt(pos).getMsgOrderFlag(); + // Undo insert()'s accounting. releaseUnitInPos() does not do it, and callers that + // release a unit account for it themselves (see readMessage, dropMessage). Without + // this the retransmission that lands in this slot is counted a second time. + countBytes(-1, -(int)packetAt(pos).getLength()); // Leaves the entry EntryState_Empty, so that a retransmission of the same // sequence number can still be inserted into this slot. releaseUnitInPos(pos); diff --git a/srtcore/core.cpp b/srtcore/core.cpp index 2d2521988..eae7c7d95 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -10631,6 +10631,9 @@ int srt::CUDT::handleSocketPacketReception(const vector& incoming, bool& const bool retransmitted = pktrexmitflag == 1; bool adding_successful = true; + // The packet arrived, but failed AEAD decryption and was erased from the RCV buffer. + // It is therefore still missing and has to be recovered like a lost packet. + bool undecrypted = false; const int32_t bufidx = CSeqNo::seqoff(bufseq, rpkt.seqno()); @@ -10739,6 +10742,7 @@ int srt::CUDT::handleSocketPacketReception(const vector& incoming, bool& if (rc != ENCS_CLEAR) { adding_successful = false; + undecrypted = true; IF_HEAVY_LOGGING(exc_type = "UNDECRYPTED"); // If TSBPD is disabled, then SRT either operates in buffer mode, of in message API without a restriction @@ -10756,7 +10760,8 @@ int srt::CUDT::handleSocketPacketReception(const vector& incoming, bool& // in the buffer. Erasing leaves the slot fillable. // The packet was added to the buffer based on the sequence number, therefore the // sequence number is used to erase it from the buffer. - // TODO: Erasing the packet means it has to be added back to the loss list. + // The erased sequence is put back into the loss list further below, so that ARQ + // retransmits a good copy of it into the slot that was just freed. const int iEraseCnt = m_pRcvBuffer->erase(u->m_Packet.getSeqNo()); const steady_clock::time_point tnow = steady_clock::now(); @@ -10766,7 +10771,7 @@ int srt::CUDT::handleSocketPacketReception(const vector& incoming, bool& if (frequentLogAllowed(FREQLOGFA_ENCRYPTION_FAILURE, tnow, (why))) { LOGC(qrlog.Warn, log << CONID() << "Decryption failed (seqno %" << u->m_Packet.getSeqNo() << "), erased " - << iEraseCnt << ". pktRcvUndecryptTotal=" << m_stats.rcvr.undecrypted.total.count() << "." << why); + << iEraseCnt << ", re-requesting. pktRcvUndecryptTotal=" << m_stats.rcvr.undecrypted.total.count() << "." << why); } #if SRT_ENABLE_FREQUENT_LOG_TRACE else @@ -10840,7 +10845,11 @@ int srt::CUDT::handleSocketPacketReception(const vector& incoming, bool& // Decryption should have made the crypto flags EK_NOENC. // Otherwise it's an error. - if (adding_successful) + // Loss detection must also run for an undecrypted packet: the sequence jump preceding it + // is real regardless of whether this particular packet could be decrypted, and + // m_iRcvCurrSeqNo advances below either way. Skipping the check would leave the jumped-over + // sequences in no loss list at all - never NAKed, and the ACK eventually passes over them. + if (adding_successful || undecrypted) { HLOGC(qrlog.Debug, log << CONID() @@ -10855,6 +10864,13 @@ int srt::CUDT::handleSocketPacketReception(const vector& incoming, bool& } } + // Re-request the erased (undecrypted) sequence so that ARQ retransmits a good copy into the + // buffer slot that erase() left fillable. This must come after the loss detection above: + // CRcvLossList::insert() rejects entries below its largest-ever sequence, so a preceding + // jump range has to be recorded first. + if (undecrypted) + w_srt_loss_seqs.push_back(make_pair(rpkt.seqno(), rpkt.seqno())); + // Update the current largest sequence number that has been received. // Or it is a retransmitted packet, remove it from receiver loss list. if (CSeqNo::seqcmp(rpkt.seqno(), m_iRcvCurrSeqNo) > 0) @@ -10867,7 +10883,13 @@ int srt::CUDT::handleSocketPacketReception(const vector& incoming, bool& } else { - unlose(rpkt); // was BELATED or RETRANSMITTED + // An undecrypted packet was erased from the buffer and is therefore still missing. + // Keep it in the receiver loss list: the ACK position (getFirstNoncontSequence) is + // derived from that list, so removing the entry would let the ACK advance past the + // erased hole, after which every retransmission of it is discarded as belated and the + // sequence can never be recovered. + if (!undecrypted) + unlose(rpkt); // was BELATED or RETRANSMITTED w_was_sent_in_order &= 0 != pktrexmitflag; } }