RTC-14408 fix ROC (#322)

RTC-14408 fix ROC
finos · Aug 16, 2023 · bd8e087 · bd8e087
1 parent 5633848
commit bd8e087
Show file tree

Hide file tree

Showing 11 changed files with 185 additions and 41 deletions.
diff --git a/bridge/endpointActions/About.cpp b/bridge/endpointActions/About.cpp
@@ -2,6 +2,9 @@
 #include "bridge/endpointActions/ApiActions.h"
 #include "config/Config.h"
 #include "git_version.h"
+#include "openssl/opensslv.h"
+#include "transport/dtls/SrtpClient.h"
+#include "utils/Format.h"
 #include "utils/StringTokenizer.h"
 
 namespace bridge
@@ -14,7 +17,10 @@ httpd::Response handleAbout(ActionContext* context,
     const auto nextToken = ::utils::StringTokenizer::tokenize(token.next, token.remainingLength, '/');
     if (utils::StringTokenizer::isEqual(nextToken, "version"))
     {
-        std::string versionString = "{\"revision\":\"" + std::string(kGitHash) + "\"}";
+        std::string versionString = utils::format(R"({"revision":"%s", "srtp":"%s", "openssl":"%s"})",
+            kGitHash,
+            srtp_get_version_string(),
+            OPENSSL_VERSION_TEXT);
         httpd::Response response(httpd::StatusCode::OK, versionString);
         response._headers["Content-type"] = "text/json";
         return response;

diff --git a/bridge/engine/AudioForwarderReceiveJob.cpp b/bridge/engine/AudioForwarderReceiveJob.cpp
@@ -116,11 +116,13 @@ void AudioForwarderReceiveJob::decode(const memory::Packet& opusPacket, memory::
 
 bool AudioForwarderReceiveJob::unprotect(memory::Packet& opusPacket)
 {
-    const auto oldRolloverCounter = _ssrcContext.lastUnprotectedExtendedSequenceNumber >> 16;
-    const auto newRolloverCounter = _extendedSequenceNumber >> 16;
-    if (newRolloverCounter > oldRolloverCounter)
+    if (transport::SrtpClient::shouldSetRolloverCounter(_ssrcContext.lastUnprotectedExtendedSequenceNumber,
+            _extendedSequenceNumber))
     {
-        logger::info("Setting new rollover counter for ssrc %u, extseqno %u->%u, seqno %u->%u, roc %u->%u, %s",
+        const uint32_t oldRolloverCounter = _ssrcContext.lastUnprotectedExtendedSequenceNumber >> 16;
+        const uint32_t newRolloverCounter = _extendedSequenceNumber >> 16;
+
+        logger::info("Setting rollover counter for ssrc %u, extseqno %u->%u, seqno %u->%u, roc %u->%u, %s",
             "AudioForwarderReceiveJob",
             _ssrcContext.ssrc,
             _ssrcContext.lastUnprotectedExtendedSequenceNumber,
@@ -147,13 +149,6 @@ bool AudioForwarderReceiveJob::unprotect(memory::Packet& opusPacket)
 
     if (!_sender->unprotect(opusPacket))
     {
-        logger::error("Failed to unprotect srtp %u, extseqno %u->%u, %s, %s",
-            "AudioForwarderReceiveJob",
-            _ssrcContext.ssrc,
-            _ssrcContext.lastUnprotectedExtendedSequenceNumber,
-            _extendedSequenceNumber,
-            _engineMixer.getLoggableId().c_str(),
-            _sender->getLoggableId().c_str());
         return false;
     }
     _ssrcContext.lastUnprotectedExtendedSequenceNumber = _extendedSequenceNumber;

diff --git a/bridge/engine/DiscardReceivedVideoPacketJob.cpp b/bridge/engine/DiscardReceivedVideoPacketJob.cpp
@@ -10,12 +10,14 @@ namespace bridge
 DiscardReceivedVideoPacketJob::DiscardReceivedVideoPacketJob(memory::UniquePacket packet,
     transport::RtcTransport* sender,
     bridge::SsrcInboundContext& ssrcContext,
-    const uint32_t extendedSequenceNumber)
+    uint32_t extendedSequenceNumber,
+    uint64_t timestamp)
     : CountedJob(sender->getJobCounter()),
       _packet(std::move(packet)),
       _sender(sender),
       _ssrcContext(ssrcContext),
-      _extendedSequenceNumber(extendedSequenceNumber)
+      _extendedSequenceNumber(extendedSequenceNumber),
+      _timestamp(timestamp)
 {
     assert(_packet);
     assert(_packet->getLength() > 0);
@@ -32,6 +34,17 @@ void DiscardReceivedVideoPacketJob::run()
         return;
     }
 
+    const bool noPacketsProcessedYet =
+        (_ssrcContext.packetsProcessed == 0 && _ssrcContext.lastReceivedExtendedSequenceNumber == 0 &&
+            _ssrcContext.lastUnprotectedExtendedSequenceNumber == 0);
+
+    if (noPacketsProcessedYet && (_extendedSequenceNumber >> 16) == 0)
+    {
+        _sender->unprotect(*_packet); // make sure srtp sees one packet with ROC=0
+        _ssrcContext.lastUnprotectedExtendedSequenceNumber = _extendedSequenceNumber;
+    }
+
+    _ssrcContext.onRtpPacketReceived(_timestamp);
     _ssrcContext.lastReceivedExtendedSequenceNumber = _extendedSequenceNumber;
     if (_ssrcContext.videoMissingPacketsTracker)
     {

diff --git a/bridge/engine/DiscardReceivedVideoPacketJob.h b/bridge/engine/DiscardReceivedVideoPacketJob.h
@@ -19,7 +19,8 @@ class DiscardReceivedVideoPacketJob : public jobmanager::CountedJob
     DiscardReceivedVideoPacketJob(memory::UniquePacket packet,
         transport::RtcTransport* sender,
         bridge::SsrcInboundContext& ssrcContext,
-        const uint32_t extendedSequenceNumber);
+        uint32_t extendedSequenceNumber,
+        uint64_t timestamp);
 
     void run() override;
 
@@ -28,6 +29,7 @@ class DiscardReceivedVideoPacketJob : public jobmanager::CountedJob
     transport::RtcTransport* _sender;
     bridge::SsrcInboundContext& _ssrcContext;
     const uint32_t _extendedSequenceNumber;
+    const uint64_t _timestamp;
 };
 
 } // namespace bridge
diff --git a/bridge/engine/EngineMixerVideo.cpp b/bridge/engine/EngineMixerVideo.cpp
@@ -411,7 +411,8 @@ void EngineMixer::onVideoRtpPacketReceived(SsrcInboundContext& ssrcContext,
         sender->getJobQueue().addJob<bridge::DiscardReceivedVideoPacketJob>(std::move(packet),
             sender,
             ssrcContext,
-            extendedSequenceNumber);
+            extendedSequenceNumber,
+            timestamp);
         return;
     }
 

diff --git a/bridge/engine/RecordingAudioForwarderSendJob.cpp b/bridge/engine/RecordingAudioForwarderSendJob.cpp
@@ -45,7 +45,7 @@ void RecordingAudioForwarderSendJob::run()
         return;
     }
 
-    uint16_t nextSequenceNumber = 0;
+    uint16_t nextSequenceNumber = 0; // TODO never set
     if (!_outboundContext.shouldSend(rtpHeader->ssrc, _extendedSequenceNumber))
     {
         logger::debug("Dropping rec audio packet - sequence number...", "RecordingAudioForwarderSendJob");

diff --git a/bridge/engine/VideoForwarderReceiveJob.cpp b/bridge/engine/VideoForwarderReceiveJob.cpp
@@ -55,14 +55,15 @@ VideoForwarderReceiveJob::VideoForwarderReceiveJob(memory::UniquePacket packet,
 
 void VideoForwarderReceiveJob::run()
 {
-    const auto oldRolloverCounter = _ssrcContext.lastUnprotectedExtendedSequenceNumber >> 16;
-    const auto newRolloverCounter = _extendedSequenceNumber >> 16;
-    if (newRolloverCounter > oldRolloverCounter)
+    if (transport::SrtpClient::shouldSetRolloverCounter(_ssrcContext.lastUnprotectedExtendedSequenceNumber,
+            _extendedSequenceNumber))
     {
-        logger::debug("Setting new rollover counter for %s, ssrc %u",
+        const uint32_t newRolloverCounter = _extendedSequenceNumber >> 16;
+        logger::info("Setting rollover counter for %s, ssrc %u, seqno %u",
             "VideoForwarderReceiveJob",
             _sender->getLoggableId().c_str(),
-            _ssrcContext.ssrc);
+            _ssrcContext.ssrc,
+            _extendedSequenceNumber);
         if (!_sender->setSrtpRemoteRolloverCounter(_ssrcContext.ssrc, newRolloverCounter))
         {
             logger::error("Failed to set rollover counter srtp %s, ssrc %u, mixer %s",
@@ -76,17 +77,6 @@ void VideoForwarderReceiveJob::run()
 
     if (!_sender->unprotect(*_packet))
     {
-        const auto header = rtp::RtpHeader::fromPacket(*_packet);
-        logger::error("Failed to unprotect srtp %s, ssrc %u, seq %u, eseq %u, lreseq %u, lueseq %u, ts %u, mixer %s",
-            "VideoForwarderReceiveJob",
-            _sender->getLoggableId().c_str(),
-            _ssrcContext.ssrc,
-            header != nullptr ? header->sequenceNumber.get() : 0,
-            _extendedSequenceNumber,
-            _ssrcContext.lastReceivedExtendedSequenceNumber,
-            _ssrcContext.lastUnprotectedExtendedSequenceNumber,
-            header != nullptr ? header->timestamp.get() : 0,
-            _engineMixer.getLoggableId().c_str());
         return;
     }
 

diff --git a/bridge/engine/VideoForwarderRtxReceiveJob.cpp b/bridge/engine/VideoForwarderRtxReceiveJob.cpp
@@ -48,11 +48,14 @@ void VideoForwarderRtxReceiveJob::run()
         return;
     }
 
-    const auto oldRolloverCounter = _rtxSsrcContext.lastUnprotectedExtendedSequenceNumber >> 16;
-    const auto newRolloverCounter = _extendedSequenceNumber >> 16;
-    if (newRolloverCounter > oldRolloverCounter)
+    if (transport::SrtpClient::shouldSetRolloverCounter(_ssrcContext.lastUnprotectedExtendedSequenceNumber,
+            _extendedSequenceNumber))
     {
-        logger::debug("Setting new rollover counter for ssrc %u", "VideoForwarderRtxReceiveJob", _rtxSsrcContext.ssrc);
+        const uint32_t newRolloverCounter = _extendedSequenceNumber >> 16;
+        logger::debug("Setting rollover counter for ssrc %u, extSeq %u",
+            "VideoForwarderRtxReceiveJob",
+            _rtxSsrcContext.ssrc,
+            _extendedSequenceNumber);
         if (!_sender->setSrtpRemoteRolloverCounter(_rtxSsrcContext.ssrc, newRolloverCounter))
         {
             logger::error("Failed to set rollover counter srtp %u, mixer %s",
@@ -65,10 +68,6 @@ void VideoForwarderRtxReceiveJob::run()
 
     if (!_sender->unprotect(*_packet))
     {
-        logger::error("Failed to unprotect srtp %u, mixer %s",
-            "VideoForwarderRtxReceiveJob",
-            _rtxSsrcContext.ssrc,
-            _engineMixer.getLoggableId().c_str());
         return;
     }
 

diff --git a/test/transport/SrtpTest.cpp b/test/transport/SrtpTest.cpp
@@ -435,3 +435,122 @@ TEST_F(SrtpTest, rocReorder)
         ASSERT_TRUE(_srtp2->unprotect(*packet));
     }
 }
+
+TEST_F(SrtpTest, receiveRocGap)
+{
+    setupDtls();
+    connect();
+
+    uint16_t seqStart = 50000;
+    uint32_t unprotectedExtSeqNo = 0;
+
+    for (uint32_t i = seqStart; i < 0x10000 * 5; ++i)
+    {
+        auto packet = memory::makeUniquePacket(_allocator, _audioPacket);
+        auto header = rtp::RtpHeader::fromPacket(*packet);
+        header->ssrc = 1;
+        header->sequenceNumber = i & 0xFFFFu;
+        header->timestamp = i * 160;
+
+        ASSERT_TRUE(_srtp1->protect(*packet));
+
+        if (i > 0x10000 + 100 && i < (0x10000 * 4 - 33000))
+        {
+            continue;
+        }
+
+        if (transport::SrtpClient::shouldSetRolloverCounter(unprotectedExtSeqNo, i))
+        {
+            logger::info("set roc %u", "", (i >> 16));
+            EXPECT_TRUE(_srtp2->setRemoteRolloverCounter(1, i >> 16)); // works without this due to continuous
+            // sequence
+        }
+        _srtp2->unprotect(*packet);
+        unprotectedExtSeqNo = i;
+    }
+}
+
+TEST_F(SrtpTest, receive2VideoGap)
+{
+    setupDtls();
+    connect();
+
+    uint16_t seqStart = 16990;
+    uint32_t unprotectedExtSeqNo = 0;
+
+    for (uint32_t i = seqStart; i < 150000; ++i)
+    {
+        auto packet = memory::makeUniquePacket(_allocator, _audioPacket);
+        auto header = rtp::RtpHeader::fromPacket(*packet);
+        header->ssrc = 1;
+        header->sequenceNumber = i & 0xFFFFu;
+        header->timestamp = i * 160;
+
+        ASSERT_TRUE(_srtp1->protect(*packet));
+        const uint32_t contPoint = 82630u;
+        // srtp must decode at least one packet while ROC is 0, otherwise we get err 7
+        if (i < 17000)
+        {
+            continue;
+        }
+        if (i > 17011 && i < contPoint)
+        {
+            continue;
+        }
+        if (i > contPoint + 2 && i < 115999)
+        {
+            continue;
+        }
+
+        if (transport::SrtpClient::shouldSetRolloverCounter(unprotectedExtSeqNo, i))
+        {
+            logger::info("set roc %u. seq %u last unprotect %u", "", (i >> 16), i, unprotectedExtSeqNo);
+            EXPECT_TRUE(_srtp2->setRemoteRolloverCounter(1, i >> 16)); // works without this due to continuous
+
+            // sequence
+        }
+        if (_srtp2->unprotect(*packet))
+        {
+            unprotectedExtSeqNo = i;
+        }
+    }
+}
+
+TEST_F(SrtpTest, muteUntilRoc1)
+{
+    setupDtls();
+    connect();
+
+    uint16_t seqStart = 65400;
+    uint32_t unprotectedExtSeqNo = 0;
+
+    for (uint32_t i = seqStart; i < 83000; ++i)
+    {
+        auto packet = memory::makeUniquePacket(_allocator, _audioPacket);
+        auto header = rtp::RtpHeader::fromPacket(*packet);
+        header->ssrc = 1;
+        header->sequenceNumber = i & 0xFFFFu;
+        header->timestamp = i * 160;
+
+        ASSERT_TRUE(_srtp1->protect(*packet));
+        const uint32_t contPoint = 82630u;
+        // srtp must decode at least one packet while ROC is 0, otherwise we get err 7
+
+        if (i < contPoint && i != seqStart)
+        {
+            continue;
+        }
+
+        if (transport::SrtpClient::shouldSetRolloverCounter(unprotectedExtSeqNo, i))
+        {
+            logger::info("set roc %u. seq %u last unprotect %u", "", (i >> 16), i, unprotectedExtSeqNo);
+            EXPECT_TRUE(_srtp2->setRemoteRolloverCounter(1, i >> 16)); // works without this due to continuous
+
+            // sequence
+        }
+        if (_srtp2->unprotect(*packet))
+        {
+            unprotectedExtSeqNo = i;
+        }
+    }
+}
diff --git a/transport/dtls/SrtpClient.cpp b/transport/dtls/SrtpClient.cpp
@@ -408,6 +408,22 @@ void SrtpClient::removeLocalSsrc(const uint32_t ssrc)
     }
 }
 
+bool SrtpClient::shouldSetRolloverCounter(uint32_t previousSequenceNumber, uint32_t sequenceNumber)
+{
+    if (!(sequenceNumber & 0xFFFF0000u) && !(previousSequenceNumber & 0xFFFF0000u))
+    {
+        return false; // cannot set ROC 0
+    }
+
+    if (previousSequenceNumber >> 15 == sequenceNumber >> 15)
+    {
+        // seqno wraps on 16 bit, but to avoid bugs after long gaps it is better to set it also after 32768
+        return false;
+    }
+
+    return true;
+}
+
 /**
  * ROC is automatically deduced from continuous inbound stream. But if there are gaps or reordering in the sequence,
  * the roc must be set explicitly.

diff --git a/transport/dtls/SrtpClient.h b/transport/dtls/SrtpClient.h
@@ -18,6 +18,8 @@ class SslWriteBioListener;
 
 // It seems sslDtls object is not thread safe when making progress in state machine during connection setup.
 // Timers, incoming packets, handshake init have to be synchronized
+// The SrtpClient must protect / unprotect at least one packet while ROC is 0. Otherwise, it will not initialize the
+// new ssrc.
 class SrtpClient : public DtlsMessageListener
 {
 public:
@@ -54,6 +56,7 @@ class SrtpClient : public DtlsMessageListener
     bool unprotect(memory::Packet& packet);
     bool protect(memory::Packet& packet);
     void removeLocalSsrc(const uint32_t ssrc);
+    static bool shouldSetRolloverCounter(uint32_t previousSequenceNumber, uint32_t sequenceNumber);
     bool setRemoteRolloverCounter(const uint32_t ssrc, const uint32_t rolloverCounter);
     bool setLocalRolloverCounter(const uint32_t ssrc, const uint32_t rolloverCounter);