Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Fixed RCV buffer initialization in Rendezvous. #2772

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

maxsharabayko
Copy link
Collaborator

@maxsharabayko maxsharabayko commented Aug 4, 2023

Initialize the receiver buffer with the peer's initial sequence number, because the peer would be sending.

Before PR #2711 the correct value was set after the RCV buffer had been initialized via CUDT::applyResponseSettings(..).

SRT versions with the bug: v1.5.2, v1.5.2.rc.2.

Fixes #2773.

@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Aug 4, 2023
@maxsharabayko maxsharabayko added this to the v1.6.0 milestone Aug 4, 2023
@maxsharabayko
Copy link
Collaborator Author

Additional changes suggested by @ethouris

diff --git a/srtcore/common.cpp b/srtcore/common.cpp
index b621c802..a194dd75 100644
--- a/srtcore/common.cpp
+++ b/srtcore/common.cpp
@@ -525,6 +525,37 @@ std::string MemberStatusStr(SRT_MEMBERSTATUS s)
 }
 #endif
 
+std::string RejectReasonStr(int r)
+{
+    static const std::string reasons[SRT_REJ_E_SIZE] = {
+         "UNKNOWN"
+        ,"SYSTEM"
+        ,"PEER"
+        ,"RESOURCE"
+        ,"ROGUE"
+        ,"BACKLOG"
+        ,"IPE"
+        ,"CLOSE"
+        ,"VERSION"
+        ,"RDVCOOKIE"
+        ,"BADSECRET"
+        ,"UNSECURE"
+        ,"MESSAGEAPI"
+        ,"CONGESTION"
+        ,"FILTER"
+        ,"GROUP"
+        ,"TIMEOUT"
+#ifdef ENABLE_AEAD_API_PREVIEW
+        ,"CRYPTO"
+#endif
+    };
+
+    if (r < int(SRT_REJ_E_SIZE))
+        return reasons[r];
+
+    return Sprint("CODE:", r);
+}
+
 // Logging system implementation
 
 #if ENABLE_LOGGING
diff --git a/srtcore/common.h b/srtcore/common.h
index 5021fa5a..ca40d20f 100644
--- a/srtcore/common.h
+++ b/srtcore/common.h
@@ -98,6 +98,8 @@ namespace srt_logging
 #if ENABLE_BONDING
     std::string MemberStatusStr(SRT_MEMBERSTATUS s);
 #endif
+
+    std::string RejectReasonStr(int r);
 }
 
 namespace srt
diff --git a/srtcore/core.cpp b/srtcore/core.cpp
index 752eb51b..b9718d21 100644
--- a/srtcore/core.cpp
+++ b/srtcore/core.cpp
@@ -3689,14 +3689,21 @@ void srt::CUDT::startConnect(const sockaddr_any& serv_addr, int32_t forced_isn)
                 if (cst == CONN_CONTINUE)
                     continue;
 
-                // Just in case it wasn't set, set this as a fallback
-                if (m_RejectReason == SRT_REJ_UNKNOWN)
-                    m_RejectReason = SRT_REJ_ROGUE;
+                HLOGC(cnlog.Debug,
+                        log << CONID() << "startConnect: processRendezvous returned cst=" << ConnectStatusStr(cst)
+                            << " rej=" << RejectReasonStr(m_RejectReason.load()));
 
-                // rejection or erroneous code.
-                reqpkt.setLength(m_iMaxSRTPayloadSize);
-                reqpkt.setControl(UMSG_HANDSHAKE);
-                sendRendezvousRejection(serv_addr, (reqpkt));
+                if (cst == CONN_REJECT)
+                {
+                    // Just in case it wasn't set, set this as a fallback
+                    if (m_RejectReason == SRT_REJ_UNKNOWN)
+                        m_RejectReason = SRT_REJ_ROGUE;
+
+                    // rejection or erroneous code.
+                    reqpkt.setLength(m_iMaxSRTPayloadSize);
+                    reqpkt.setControl(UMSG_HANDSHAKE);
+                    sendRendezvousRejection(serv_addr, (reqpkt));
+                }
             }
 
             if (cst == CONN_REJECT)
@@ -4679,7 +4686,8 @@ bool srt::CUDT::applyResponseSettings(const CPacket* pHspkt /*[[nullable]]*/) AT
 
     HLOGC(cnlog.Debug,
           log << CONID() << "applyResponseSettings: HANSHAKE CONCLUDED. SETTING: payload-size=" << m_iMaxSRTPayloadSize
-              << " mss=" << m_ConnRes.m_iMSS << " flw=" << m_ConnRes.m_iFlightFlagSize << " isn=" << m_ConnRes.m_iISN
+              << " mss=" << m_ConnRes.m_iMSS << " flw=" << m_ConnRes.m_iFlightFlagSize << " peer-ISN=" << m_ConnRes.m_iISN
+              << " local-ISN=" << m_iISN
               << " peerID=" << m_ConnRes.m_iID
               << " sourceIP=" << m_SourceAddr.str());
     return true;

@ethouris
Copy link
Collaborator

ethouris commented Aug 7, 2023

My proposal actually fixes another problem in rendezvous which was done due to an incorrect fix some time ago, where the rendezvous rejection wasn't properly handled. Important is to add this check if the returned cst is CONN_REJECT as only in this case should the rejection be sent.

@maxsharabayko maxsharabayko merged commit 69c2376 into Haivision:master Aug 7, 2023
9 checks passed
@maxsharabayko maxsharabayko deleted the hotfix/rendezvous branch August 7, 2023 15:00
@maxsharabayko
Copy link
Collaborator Author

@ethouris Please create a separate PR from your proposal.

@maxsharabayko maxsharabayko modified the milestones: v1.6.0, v1.5.3 Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rendezvous Connection: Wrong Initial Sequence Initialization of the Receiver
2 participants